[PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Please note following potential bug was discovered by code review, and
my patch not even tested, please double check !

Thanks

[PATCH net-next-2.6] netlink: netlink_recvmsg() fix

commit 1dacc76d0014 
(net/compat/wext: send different messages to compat tasks)
introduced a race condition on netlink, in case MSG_PEEK is used.

An skb given by skb_recv_datagram() might be shared, we must clone it
before any modification, or risk fatal corruption.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/netlink/af_netlink.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7aeaa83..dad5e81 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1405,7 +1405,7 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	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;
 	int err;
 
 	if (flags&MSG_OOB)
@@ -1440,8 +1440,17 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 			kfree_skb(skb);
 			skb = compskb;
 		} else {
-			frag = skb_shinfo(skb)->frag_list;
-			skb_shinfo(skb)->frag_list = NULL;
+			struct sk_buff *nskb = skb_clone(skb, GFP_KERNEL);
+
+			if (!nskb) {
+				skb_free_datagram(sk, skb);
+				err = -ENOMEM;
+				goto out;
+			}
+			kfree_skb(skb);
+			kfree_skb(skb_shinfo(nskb)->frag_list);
+			skb_shinfo(nskb)->frag_list = NULL;
+			skb = nskb;
 		}
 	}
 #endif
@@ -1477,10 +1486,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	if (flags & MSG_TRUNC)
 		copied = skb->len;
 
-#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
-	skb_shinfo(skb)->frag_list = frag;
-#endif
-
 	skb_free_datagram(sk, skb);
 
 	if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le mardi 20 juillet 2010 à 15:16 +0200, Eric Dumazet a écrit :
> Please note following potential bug was discovered by code review, and
> my patch not even tested, please double check !
> 
> Thanks
> 
> [PATCH net-next-2.6] netlink: netlink_recvmsg() fix
> 
> commit 1dacc76d0014 
> (net/compat/wext: send different messages to compat tasks)
> introduced a race condition on netlink, in case MSG_PEEK is used.
> 
> An skb given by skb_recv_datagram() might be shared, we must clone it
> before any modification, or risk fatal corruption.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---

Oh well, skb_copy() or skb_unshare() is needed.

[PATCH net-next-2.6 v2] netlink: netlink_recvmsg() fix

commit 1dacc76d0014 
(net/compat/wext: send different messages to compat tasks)
introduced a race condition on netlink, in case MSG_PEEK is used.

An skb given by skb_recv_datagram() might be shared, we must copy it
before any modification, or risk fatal corruption.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/netlink/af_netlink.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7aeaa83..1537fa5 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1405,7 +1405,7 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	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;
 	int err;
 
 	if (flags&MSG_OOB)
@@ -1440,7 +1440,12 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 			kfree_skb(skb);
 			skb = compskb;
 		} else {
-			frag = skb_shinfo(skb)->frag_list;
+			skb = skb_unshare(skb, GFP_KERNEL);
+			if (!skb) {
+				err = -ENOMEM;
+				goto out;
+			}
+			kfree_skb(skb_shinfo(skb)->frag_list);
 			skb_shinfo(skb)->frag_list = NULL;
 		}
 	}
@@ -1477,10 +1482,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	if (flags & MSG_TRUNC)
 		copied = skb->len;
 
-#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
-	skb_shinfo(skb)->frag_list = frag;
-#endif
-
 	skb_free_datagram(sk, skb);
 
 	if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Tue, 2010-07-20 at 17:20 +0200, Eric Dumazet wrote:

> [PATCH net-next-2.6 v2] netlink: netlink_recvmsg() fix
> 
> commit 1dacc76d0014 
> (net/compat/wext: send different messages to compat tasks)
> introduced a race condition on netlink, in case MSG_PEEK is used.
> 
> An skb given by skb_recv_datagram() might be shared, we must copy it
> before any modification, or risk fatal corruption.

Makes sense to me, seeing that if you MSG_PEEK it just increases
skb->users. But nothing could touch the other skb at the same time?
Although I guess with netlink multicast we have a similar situation.

johannes

> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/netlink/af_netlink.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 7aeaa83..1537fa5 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1405,7 +1405,7 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
>  	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;
>  	int err;
>  
>  	if (flags&MSG_OOB)
> @@ -1440,7 +1440,12 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
>  			kfree_skb(skb);
>  			skb = compskb;
>  		} else {
> -			frag = skb_shinfo(skb)->frag_list;
> +			skb = skb_unshare(skb, GFP_KERNEL);
> +			if (!skb) {
> +				err = -ENOMEM;
> +				goto out;
> +			}
> +			kfree_skb(skb_shinfo(skb)->frag_list);
>  			skb_shinfo(skb)->frag_list = NULL;
>  		}
>  	}
> @@ -1477,10 +1482,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
>  	if (flags & MSG_TRUNC)
>  		copied = skb->len;
>  
> -#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
> -	skb_shinfo(skb)->frag_list = frag;
> -#endif
> -
>  	skb_free_datagram(sk, skb);
>  
>  	if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)
> 
> 
> 

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le mercredi 21 juillet 2010 à 10:05 +0200, Johannes Berg a écrit :
> On Tue, 2010-07-20 at 17:20 +0200, Eric Dumazet wrote:
> 
> > [PATCH net-next-2.6 v2] netlink: netlink_recvmsg() fix
> > 
> > commit 1dacc76d0014 
> > (net/compat/wext: send different messages to compat tasks)
> > introduced a race condition on netlink, in case MSG_PEEK is used.
> > 
> > An skb given by skb_recv_datagram() might be shared, we must copy it
> > before any modification, or risk fatal corruption.
> 
> Makes sense to me, seeing that if you MSG_PEEK it just increases
> skb->users. But nothing could touch the other skb at the same time?
> Although I guess with netlink multicast we have a similar situation.

Nothing can touch this skb at the same time but us and our friends
(consumers that did a skb_recv_datagram( MSG_PEEK ) operation).

Oh well, I see skb_unshare() tests skb_cloned(). This is not what we
want.

We probably wants something like :

if (skb_shared(skb)) {
	nsbk = skb_copy(skb, GFP_KERNEL);
	...
}

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le mercredi 21 juillet 2010 à 10:20 +0200, Eric Dumazet a écrit :

> Nothing can touch this skb at the same time but us and our friends
> (consumers that did a skb_recv_datagram( MSG_PEEK ) operation).
> 
> Oh well, I see skb_unshare() tests skb_cloned(). This is not what we
> want.
> 

Here is V3 of this patch.

Thanks

[PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix

commit 1dacc76d0014 
(net/compat/wext: send different messages to compat tasks)
introduced a race condition on netlink, in case MSG_PEEK is used.

An skb given by skb_recv_datagram() might be shared, we must copy it
before any modification, or risk fatal corruption.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/netlink/af_netlink.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7aeaa83..0c86657 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1405,7 +1405,7 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	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;
 	int err;
 
 	if (flags&MSG_OOB)
@@ -1440,7 +1440,21 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 			kfree_skb(skb);
 			skb = compskb;
 		} else {
-			frag = skb_shinfo(skb)->frag_list;
+			/*
+			 * Before setting frag_list to NULL, we must get a
+			 * private copy of skb if shared (because of MSG_PEEK)
+			 */
+			if (skb_shared(skb) {
+				struct sk_buff *nskb;
+
+				nskb = pskb_copy(skb, GFP_KERNEL);
+				kfree_skb(skb);
+				skb = nskb;
+				err = -ENOMEM;
+				if (!skb)
+					goto out;
+			}
+			kfree_skb(skb_shinfo(skb)->frag_list);
 			skb_shinfo(skb)->frag_list = NULL;
 		}
 	}
@@ -1477,10 +1491,6 @@ static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
 	if (flags & MSG_TRUNC)
 		copied = skb->len;
 
-#ifdef CONFIG_COMPAT_NETLINK_MESSAGES
-	skb_shinfo(skb)->frag_list = frag;
-#endif
-
 	skb_free_datagram(sk, skb);
 
 	if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 21 Jul 2010 10:43:55 +0200

> [PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix
> 
> commit 1dacc76d0014 
> (net/compat/wext: send different messages to compat tasks)
> introduced a race condition on netlink, in case MSG_PEEK is used.
> 
> An skb given by skb_recv_datagram() might be shared, we must copy it
> before any modification, or risk fatal corruption.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks Eric.

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sun, 25 Jul 2010 21:55:48 -0700 (PDT)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 21 Jul 2010 10:43:55 +0200
> 
>> [PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix
>> 
>> commit 1dacc76d0014 
>> (net/compat/wext: send different messages to compat tasks)
>> introduced a race condition on netlink, in case MSG_PEEK is used.
>> 
>> An skb given by skb_recv_datagram() might be shared, we must copy it
>> before any modification, or risk fatal corruption.
>> 
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Applied, thanks Eric.

I bet you didn't compile test the code you modified at all, but it's
not your fault :-)

The code is protected by CONFIG_WIRELESS_EXT but that protection is
not valid.  It should be protected by something like CONFIG_WEXT_CORE
or similar.

The only way to get CONFIG_WIRELESS_EXT set it to enable one of a few
drivers, many of which are in staging.

Anyways, just a heads up, I'll fix this up.

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le lundi 26 juillet 2010 à 13:08 -0700, David Miller a écrit :

> I bet you didn't compile test the code you modified at all, but it's
> not your fault :-)
> 
> The code is protected by CONFIG_WIRELESS_EXT but that protection is
> not valid.  It should be protected by something like CONFIG_WEXT_CORE
> or similar.
> 
> The only way to get CONFIG_WIRELESS_EXT set it to enable one of a few
> drivers, many of which are in staging.
> 
> Anyways, just a heads up, I'll fix this up.

Well, I recall having compiled (but not boot tested) patches,
on my x86_64 dev machine with a .config containing :

# grep WIRELESS_EXT .config
CONFIG_WIRELESS_EXT=y
CONFIG_WIRELESS_EXT_SYSFS=y
# grep COMPAT_NETLINK_MESSAGES .config
CONFIG_COMPAT_NETLINK_MESSAGES=y

Cannot remember if I compiled v3 of the patch, to be honest.

Thanks

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 26 Jul 2010 22:39:03 +0200

> Cannot remember if I compiled v3 of the patch, to be honest.

It's impossible that you did so, there is a missing
closing parenthesis in the first if() statement added
by your patch :-)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le lundi 26 juillet 2010 à 13:48 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 26 Jul 2010 22:39:03 +0200
> 
> > Cannot remember if I compiled v3 of the patch, to be honest.
> 
> It's impossible that you did so, there is a missing
> closing parenthesis in the first if() statement added
> by your patch :-)

Oops

Yes, I realized this shortly after sending my previous mail :)

Thanks

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Sun, 2010-07-25 at 21:55 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 21 Jul 2010 10:43:55 +0200
> 
> > [PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix
> > 
> > commit 1dacc76d0014 
> > (net/compat/wext: send different messages to compat tasks)
> > introduced a race condition on netlink, in case MSG_PEEK is used.
> > 
> > An skb given by skb_recv_datagram() might be shared, we must copy it
> > before any modification, or risk fatal corruption.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Applied, thanks Eric.

I keep getting errors like below in 2.6.35+wireless-testing. Not saying
that it's this patch's fault, but it is the only thing I remember
touching that area.

[10548.768754] =============================================================================
[10548.773474] BUG kmalloc-4096: Object already free
[10548.775342] -----------------------------------------------------------------------------
[10548.775344] 
[10548.778505] INFO: Allocated in wireless_send_event+0x1f2/0x410 age=1 cpu=0 pid=28521
[10548.778505] INFO: Freed in skb_release_data+0xd0/0xe0 age=2 cpu=2 pid=27730
[10548.778505] INFO: Slab 0xffffea0003fa4b80 objects=7 used=1 fp=0xffff880122f12090 flags=0x80000000000040c3
[10548.778505] INFO: Object 0xffff880122f12090 @offset=8336 fp=0xffff880122f11048
[10548.778505] 
[10548.778505] Bytes b4 0xffff880122f12080:  6c a3 0f 00 01 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a l�......ZZZZZZZZ
...
[10548.778505]   Object 0xffff880122f13080:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk�
[10548.778505]  Redzone 0xffff880122f13090:  bb bb bb bb bb bb bb bb                         ��������        
[10548.778505]  Padding 0xffff880122f130d0:  5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ        
[10548.778505] Pid: 916, comm: avahi-daemon Tainted: G        W   2.6.35-wl-67184-g1595c70-dirty #12
[10548.778505] Call Trace:
[10548.778505]  [<ffffffff81133993>] print_trailer+0x103/0x160
[10548.778505]  [<ffffffff81133a31>] object_err+0x41/0x50
[10548.778505]  [<ffffffff81135b0f>] __slab_free+0x24f/0x3b0
[10548.778505]  [<ffffffff81135da2>] kfree+0x132/0x1a0
[10548.778505]  [<ffffffff81383710>] skb_release_data+0xd0/0xe0
[10548.778505]  [<ffffffff813831be>] __kfree_skb+0x1e/0xa0
[10548.778505]  [<ffffffff813832c2>] kfree_skb+0x42/0xb0
[10548.778505]  [<ffffffff813ac3f2>] netlink_recvmsg+0x422/0x480
[10548.778505]  [<ffffffff8137afdd>] sock_recvmsg+0xfd/0x130
[10548.778505]  [<ffffffff8137c1e4>] __sys_recvmsg+0x144/0x2e0
[10548.778505]  [<ffffffff8137c629>] sys_recvmsg+0x49/0x80
[10548.778505]  [<ffffffff8100b132>] system_call_fastpath+0x16/0x1b
[10548.778505] FIX kmalloc-4096: Object at 0xffff880122f12090 not freed


johannes

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Fri, 2010-08-13 at 16:00 +0200, Johannes Berg wrote:
> On Sun, 2010-07-25 at 21:55 -0700, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, 21 Jul 2010 10:43:55 +0200
> > 
> > > [PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix
> > > 
> > > commit 1dacc76d0014 
> > > (net/compat/wext: send different messages to compat tasks)
> > > introduced a race condition on netlink, in case MSG_PEEK is used.
> > > 
> > > An skb given by skb_recv_datagram() might be shared, we must copy it
> > > before any modification, or risk fatal corruption.
> > > 
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > 
> > Applied, thanks Eric.
> 
> I keep getting errors like below in 2.6.35+wireless-testing. Not saying
> that it's this patch's fault, but it is the only thing I remember
> touching that area.

In fact, I think something's wrong with this patch, since my comment
(that right now unfortunately I no longer fully understand) says:

                 * 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.
                 *
                 * 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.

and that's no longer true, afaict.

johannes

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le vendredi 13 août 2010 à 16:35 +0200, Johannes Berg a écrit :
> On Fri, 2010-08-13 at 16:00 +0200, Johannes Berg wrote:
> > On Sun, 2010-07-25 at 21:55 -0700, David Miller wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Wed, 21 Jul 2010 10:43:55 +0200
> > > 
> > > > [PATCH net-next-2.6 v3] netlink: netlink_recvmsg() fix
> > > > 
> > > > commit 1dacc76d0014 
> > > > (net/compat/wext: send different messages to compat tasks)
> > > > introduced a race condition on netlink, in case MSG_PEEK is used.
> > > > 
> > > > An skb given by skb_recv_datagram() might be shared, we must copy it
> > > > before any modification, or risk fatal corruption.
> > > > 
> > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > 
> > > Applied, thanks Eric.
> > 
> > I keep getting errors like below in 2.6.35+wireless-testing. Not saying
> > that it's this patch's fault, but it is the only thing I remember
> > touching that area.
> 
> In fact, I think something's wrong with this patch, since my comment
> (that right now unfortunately I no longer fully understand) says:
> 
>                  * 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.
>                  *
>                  * 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.
> 
> and that's no longer true, afaict.
> 

Comment was not updated by the patch.

But do you agree temporarly setting frag_list to NULL was a bug ?

Unfortunatly I cannot test this path...

I assume reverting 1235f504aaba removes these errors ?


Its strange we have a double-free on a data part and not a skb_head.

maybe pskb_copy() has a problem with frag_list...


Maybe we can revert the patch and find another way to make sure two
process can not manipulate this skb in // (adding a mutex, or using
RTNL ?)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Fri, 2010-08-13 at 16:48 +0200, Eric Dumazet wrote:

> >                  * 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.
> >                  *
> >                  * 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.
> > 
> > and that's no longer true, afaict.
> > 
> 
> Comment was not updated by the patch.
> 
> But do you agree temporarly setting frag_list to NULL was a bug ?

No, that was actually intentional, as the comment describes?

> Unfortunatly I cannot test this path...

No wireless devices? :)

> I assume reverting 1235f504aaba removes these errors ?

I haven't tried yet, but it only happened very recently and I didn't
find any other candidate -- the error always points to
wireless_send_event too.

> Its strange we have a double-free on a data part and not a skb_head.

There also was an apparent use-after-free error.

> maybe pskb_copy() has a problem with frag_list...
> 
> 
> Maybe we can revert the patch and find another way to make sure two
> process can not manipulate this skb in // (adding a mutex, or using
> RTNL ?)

I'm getting confused. Why did I think the NULLing and then restoring was
not racy to start with?

johannes

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 13 Aug 2010 17:13:53 +0200

> On Fri, 2010-08-13 at 16:48 +0200, Eric Dumazet wrote:
> 
>> I assume reverting 1235f504aaba removes these errors ?
> 
> I haven't tried yet, but it only happened very recently and I didn't
> find any other candidate -- the error always points to
> wireless_send_event too.

Please test with the commit reverted and let us know if it helps.

The current situation is worse than what we were trying to fix
in that commit, so if a revert fixes your problem then as Eric
said we should do that first.

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Sat, 2010-08-14 at 22:37 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 13 Aug 2010 17:13:53 +0200
> 
> > On Fri, 2010-08-13 at 16:48 +0200, Eric Dumazet wrote:
> > 
> >> I assume reverting 1235f504aaba removes these errors ?
> > 
> > I haven't tried yet, but it only happened very recently and I didn't
> > find any other candidate -- the error always points to
> > wireless_send_event too.
> 
> Please test with the commit reverted and let us know if it helps.
> 
> The current situation is worse than what we were trying to fix
> in that commit, so if a revert fixes your problem then as Eric
> said we should do that first.

I haven't gotten around to it, but Kalle had been running into the same
issue and said reverting it fixed it:

http://article.gmane.org/gmane.linux.kernel.wireless.general/54492

johannes

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le lundi 16 août 2010 à 07:25 +0200, Johannes Berg a écrit :

> I haven't gotten around to it, but Kalle had been running into the same
> issue and said reverting it fixed it:
> 
> http://article.gmane.org/gmane.linux.kernel.wireless.general/54492
> 

Thanks Johannes

Please David revert 1235f504.

We'll find another way to address the problem.

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Aug 2010 08:10:02 +0200

> Le lundi 16 août 2010 à 07:25 +0200, Johannes Berg a écrit :
> 
>> I haven't gotten around to it, but Kalle had been running into the same
>> issue and said reverting it fixed it:
>> 
>> http://article.gmane.org/gmane.linux.kernel.wireless.general/54492
>> 
> 
> Thanks Johannes
> 
> Please David revert 1235f504.
> 
> We'll find another way to address the problem.

I will, thanks guys.

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Mon, 2010-08-16 at 08:10 +0200, Eric Dumazet wrote:

> We'll find another way to address the problem.

For my understanding: The problem is that the save/restore of the
frag_list skb can race so a second reader will not see the frag_list skb
because it gets there while it's NULL. This happens with MSG_PEEK, or
with clones of the SKB since skb_shared_info is invariant across clones.

Correct?

johannes

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Eric Dumazet]

Le lundi 16 août 2010 à 08:22 +0200, Johannes Berg a écrit :
> On Mon, 2010-08-16 at 08:10 +0200, Eric Dumazet wrote:
> 
> > We'll find another way to address the problem.
> 
> For my understanding: The problem is that the save/restore of the
> frag_list skb can race so a second reader will not see the frag_list skb
> because it gets there while it's NULL. This happens with MSG_PEEK, or
> with clones of the SKB since skb_shared_info is invariant across clones.
> 
> Correct?
> 

Yes

I believe we should have a mutual exclusion for the critical section.

(setting pointer to NULL, ...., setting pointer back to its orig value)

Re: [PATCH net-next-2.6] netlink: netlink_recvmsg() fix

[Johannes Berg]

On Mon, 2010-08-16 at 08:29 +0200, Eric Dumazet wrote:
> Le lundi 16 août 2010 à 08:22 +0200, Johannes Berg a écrit :
> > On Mon, 2010-08-16 at 08:10 +0200, Eric Dumazet wrote:
> > 
> > > We'll find another way to address the problem.
> > 
> > For my understanding: The problem is that the save/restore of the
> > frag_list skb can race so a second reader will not see the frag_list skb
> > because it gets there while it's NULL. This happens with MSG_PEEK, or
> > with clones of the SKB since skb_shared_info is invariant across clones.
> > 
> > Correct?
> > 
> 
> Yes
> 
> I believe we should have a mutual exclusion for the critical section.
> 
> (setting pointer to NULL, ...., setting pointer back to its orig value)

Yeah, that'd work, but I'm wondering now why I did this at all. The code
already restricts the data copying to skb->len, which will not include
the frag_list, so it should be OK to just leave it intact? I'll go and
look at that in more detail and try it.

johannes

[PATCH] netlink: fix compat recvmsg

[Johannes Berg]

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>
---
 net/netlink/af_netlink.c |   46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

--- 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);
 

Re: [PATCH] netlink: fix compat recvmsg

[Eric Dumazet]

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 !

Re: [PATCH] netlink: fix compat recvmsg

[Johannes Berg]

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

Re: [PATCH] netlink: fix compat recvmsg

[Eric Dumazet]

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 ;)

Re: [PATCH] netlink: fix compat recvmsg

[David Miller]

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!