| Message ID | 1281943244.3683.8.camel@jlt3.sipsolutions.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 16 août 2010 à 09:20 +0200, Johannes Berg a écrit : > From: Johannes Berg <johannes.berg@intel.com> > > Since > commit 1dacc76d0014a034b8aca14237c127d7c19d7726 > Author: Johannes Berg <johannes@sipsolutions.net> > Date: Wed Jul 1 11:26:02 2009 +0000 > > net/compat/wext: send different messages to compat tasks > > we had a race condition when setting and then > restoring frag_list. Eric attempted to fix it, > but the fix created even worse problems. > > However, the original motivation I had when I > added the code that turned out to be racy is > no longer clear to me, since we only copy up > to skb->len to userspace, which doesn't include > the frag_list length. As a result, not doing > any frag_list clearing and restoring avoids > the race condition, while not introducing any > other problems. > > Additionally, while preparing this patch I found > that since none of the remaining netlink code is > really aware of the frag_list, we need to use the > original skb's information for packet information > and credentials. This fixes, for example, the > group information received by compat tasks. > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: stable@kernel.org [2.6.31+, for 2.6.35 revert 1235f504aa] > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- As a side effect, it might fix the if (nlk->flags & NETLINK_RECV_PKTINFO) netlink_cmsg_recv_pktinfo(msg, skb); That tried to use pktinfo from the slave skb. After this patch, skb points to master skb. Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2010-08-16 at 14:50 +0200, Eric Dumazet wrote: > > Additionally, while preparing this patch I found > > that since none of the remaining netlink code is > > really aware of the frag_list, we need to use the > > original skb's information for packet information > > and credentials. This fixes, for example, the > > group information received by compat tasks. > As a side effect, it might fix the > > if (nlk->flags & NETLINK_RECV_PKTINFO) > netlink_cmsg_recv_pktinfo(msg, skb); > > That tried to use pktinfo from the slave skb. After this > patch, skb points to master skb. Yeah, that's what I was looking at regarding the "packet information" I wrote about. The group information was wrong (0 rather than 1), I could see that in strace (though I had to compile a strace64 binary first...) > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> > > Thanks ! Thanks for looking, and finding the bug to start with :-) johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 16 août 2010 à 14:50 +0200, Eric Dumazet a écrit : > Le lundi 16 août 2010 à 09:20 +0200, Johannes Berg a écrit : > > From: Johannes Berg <johannes.berg@intel.com> > > > > Since > > commit 1dacc76d0014a034b8aca14237c127d7c19d7726 > > Author: Johannes Berg <johannes@sipsolutions.net> > > Date: Wed Jul 1 11:26:02 2009 +0000 > > > > net/compat/wext: send different messages to compat tasks > > > > we had a race condition when setting and then > > restoring frag_list. Eric attempted to fix it, > > but the fix created even worse problems. > > > > However, the original motivation I had when I > > added the code that turned out to be racy is > > no longer clear to me, since we only copy up > > to skb->len to userspace, which doesn't include > > the frag_list length. As a result, not doing > > any frag_list clearing and restoring avoids > > the race condition, while not introducing any > > other problems. > > > > Additionally, while preparing this patch I found > > that since none of the remaining netlink code is > > really aware of the frag_list, we need to use the > > original skb's information for packet information > > and credentials. This fixes, for example, the > > group information received by compat tasks. > > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > Cc: stable@kernel.org [2.6.31+, for 2.6.35 revert 1235f504aa] > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > --- > > As a side effect, it might fix the > > if (nlk->flags & NETLINK_RECV_PKTINFO) > netlink_cmsg_recv_pktinfo(msg, skb); > > That tried to use pktinfo from the slave skb. After this > patch, skb points to master skb. > As you already mentioned it in your changelog, I missed it ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 16 Aug 2010 14:50:25 +0200 > Le lundi 16 août 2010 à 09:20 +0200, Johannes Berg a écrit : >> From: Johannes Berg <johannes.berg@intel.com> >> >> Since >> commit 1dacc76d0014a034b8aca14237c127d7c19d7726 >> Author: Johannes Berg <johannes@sipsolutions.net> >> Date: Wed Jul 1 11:26:02 2009 +0000 >> >> net/compat/wext: send different messages to compat tasks >> >> we had a race condition when setting and then >> restoring frag_list. Eric attempted to fix it, >> but the fix created even worse problems. ... >> Signed-off-by: Johannes Berg <johannes.berg@intel.com> ... > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks guys! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- wireless-testing.orig/net/netlink/af_netlink.c 2010-08-16 08:33:42.000000000 +0200 +++ wireless-testing/net/netlink/af_netlink.c 2010-08-16 09:13:17.000000000 +0200 @@ -1406,7 +1406,7 @@ static int netlink_recvmsg(struct kiocb struct netlink_sock *nlk = nlk_sk(sk); int noblock = flags&MSG_DONTWAIT; size_t copied; - struct sk_buff *skb, *frag __maybe_unused = NULL; + struct sk_buff *skb, *data_skb; int err; if (flags&MSG_OOB) @@ -1418,45 +1418,35 @@ static int netlink_recvmsg(struct kiocb if (skb == NULL) goto out; + data_skb = skb; + #ifdef CONFIG_COMPAT_NETLINK_MESSAGES if (unlikely(skb_shinfo(skb)->frag_list)) { - bool need_compat = !!(flags & MSG_CMSG_COMPAT); - /* - * If this skb has a frag_list, then here that means that - * we will have to use the frag_list skb for compat tasks - * and the regular skb for non-compat tasks. + * If this skb has a frag_list, then here that means that we + * will have to use the frag_list skb's data for compat tasks + * and the regular skb's data for normal (non-compat) tasks. * - * The skb might (and likely will) be cloned, so we can't - * just reset frag_list and go on with things -- we need to - * keep that. For the compat case that's easy -- simply get - * a reference to the compat skb and free the regular one - * including the frag. For the non-compat case, we need to - * avoid sending the frag to the user -- so assign NULL but - * restore it below before freeing the skb. + * If we need to send the compat skb, assign it to the + * 'data_skb' variable so that it will be used below for data + * copying. We keep 'skb' for everything else, including + * freeing both later. */ - if (need_compat) { - struct sk_buff *compskb = skb_shinfo(skb)->frag_list; - skb_get(compskb); - kfree_skb(skb); - skb = compskb; - } else { - frag = skb_shinfo(skb)->frag_list; - skb_shinfo(skb)->frag_list = NULL; - } + if (flags & MSG_CMSG_COMPAT) + data_skb = skb_shinfo(skb)->frag_list; } #endif msg->msg_namelen = 0; - copied = skb->len; + copied = data_skb->len; if (len < copied) { msg->msg_flags |= MSG_TRUNC; copied = len; } - skb_reset_transport_header(skb); - err = skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied); + skb_reset_transport_header(data_skb); + err = skb_copy_datagram_iovec(data_skb, 0, msg->msg_iov, copied); if (msg->msg_name) { struct sockaddr_nl *addr = (struct sockaddr_nl *)msg->msg_name; @@ -1476,11 +1466,7 @@ static int netlink_recvmsg(struct kiocb } siocb->scm->creds = *NETLINK_CREDS(skb); if (flags & MSG_TRUNC) - copied = skb->len; - -#ifdef CONFIG_COMPAT_NETLINK_MESSAGES - skb_shinfo(skb)->frag_list = frag; -#endif + copied = data_skb->len; skb_free_datagram(sk, skb);