diff mbox series

[v2,3/6] netlink/compat: Append NLMSG_DONE/extack to frag_list

Message ID 20200826014949.644441-4-dima@arista.com
State Awaiting Upstream
Delegated to: David Miller
Headers show
Series xfrm: Add compat layer | expand

Commit Message

Dmitry Safonov Aug. 26, 2020, 1:49 a.m. UTC 
Modules those use netlink may supply a 2nd skb, (via frag_list)
that contains an alternative data set meant for applications
using 32bit compatibility mode.

In such a case, netlink_recvmsg will use this 2nd skb instead of the
original one.

Without this patch, such compat applications will retrieve
all netlink dump data, but will then get an unexpected EOF.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 net/netlink/af_netlink.c | 48 ++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 14 deletions(-)

Comments

Johannes Berg Aug. 26, 2020, 7:19 a.m. UTC | #1 
On Wed, 2020-08-26 at 02:49 +0100, Dmitry Safonov wrote:
> Modules those use netlink may supply a 2nd skb, (via frag_list)
> that contains an alternative data set meant for applications
> using 32bit compatibility mode.

Do note, however, that until this day the facility here was only used by
(and originally intended for) wireless extensions, where it exclusively
applies to *event* messages. Hence, we really didn't worry about dump or
any other kind of netlink usage.

That said, it's really just a historic note explaining why it didn't
work for you out of the box :)

> In such a case, netlink_recvmsg will use this 2nd skb instead of the
> original one.
> 
> Without this patch, such compat applications will retrieve
> all netlink dump data, but will then get an unexpected EOF.
> 
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  net/netlink/af_netlink.c | 48 ++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index b5f30d7d30d0..b096f2b4a50d 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2186,13 +2186,36 @@ EXPORT_SYMBOL(__nlmsg_put);
>   * It would be better to create kernel thread.
>   */
>  
> +static int netlink_dump_done(struct netlink_sock *nlk, struct sk_buff *skb,
> +			     struct netlink_callback *cb,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(nlk->dump_done_errno),
> +			       NLM_F_MULTI | cb->answer_flags);
> +	if (WARN_ON(!nlh))
> +		return -ENOBUFS;
> +
> +	nl_dump_check_consistent(cb, nlh);
> +	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno,
> +			sizeof(nlk->dump_done_errno));

nit: indentation here looks odd.

Other than that, looks reasonable to me.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes
Dmitry Safonov Sept. 7, 2020, 4:41 p.m. UTC | #2 
On 8/26/20 8:19 AM, Johannes Berg wrote:
> On Wed, 2020-08-26 at 02:49 +0100, Dmitry Safonov wrote:
[..]
>> +	nl_dump_check_consistent(cb, nlh);
>> +	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno,
>> +			sizeof(nlk->dump_done_errno));
> 
> nit: indentation here looks odd.
> 
> Other than that, looks reasonable to me.
> 
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Thank you for the review!
diff mbox series

Patch

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index b5f30d7d30d0..b096f2b4a50d 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2186,13 +2186,36 @@  EXPORT_SYMBOL(__nlmsg_put);
  * It would be better to create kernel thread.
  */
 
+static int netlink_dump_done(struct netlink_sock *nlk, struct sk_buff *skb,
+			     struct netlink_callback *cb,
+			     struct netlink_ext_ack *extack)
+{
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(nlk->dump_done_errno),
+			       NLM_F_MULTI | cb->answer_flags);
+	if (WARN_ON(!nlh))
+		return -ENOBUFS;
+
+	nl_dump_check_consistent(cb, nlh);
+	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno,
+			sizeof(nlk->dump_done_errno));
+
+	if (extack->_msg && nlk->flags & NETLINK_F_EXT_ACK) {
+		nlh->nlmsg_flags |= NLM_F_ACK_TLVS;
+		if (!nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
+			nlmsg_end(skb, nlh);
+	}
+
+	return 0;
+}
+
 static int netlink_dump(struct sock *sk)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
 	struct netlink_ext_ack extack = {};
 	struct netlink_callback *cb;
 	struct sk_buff *skb = NULL;
-	struct nlmsghdr *nlh;
 	struct module *module;
 	int err = -ENOBUFS;
 	int alloc_min_size;
@@ -2258,22 +2281,19 @@  static int netlink_dump(struct sock *sk)
 		return 0;
 	}
 
-	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE,
-			       sizeof(nlk->dump_done_errno),
-			       NLM_F_MULTI | cb->answer_flags);
-	if (WARN_ON(!nlh))
+	if (netlink_dump_done(nlk, skb, cb, &extack))
 		goto errout_skb;
 
-	nl_dump_check_consistent(cb, nlh);
-
-	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno,
-	       sizeof(nlk->dump_done_errno));
-
-	if (extack._msg && nlk->flags & NETLINK_F_EXT_ACK) {
-		nlh->nlmsg_flags |= NLM_F_ACK_TLVS;
-		if (!nla_put_string(skb, NLMSGERR_ATTR_MSG, extack._msg))
-			nlmsg_end(skb, nlh);
+#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
+	/* frag_list skb's data is used for compat tasks
+	 * and the regular skb's data for normal (non-compat) tasks.
+	 * See netlink_recvmsg().
+	 */
+	if (unlikely(skb_shinfo(skb)->frag_list)) {
+		if (netlink_dump_done(nlk, skb_shinfo(skb)->frag_list, cb, &extack))
+			goto errout_skb;
 	}
+#endif
 
 	if (sk_filter(sk, skb))
 		kfree_skb(skb);