* [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
@ 2013-08-19 7:05 Dan Carpenter
2013-08-19 7:33 ` Nicolas Dichtel
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dan Carpenter @ 2013-08-19 7:05 UTC (permalink / raw)
To: David S. Miller, Nicolas Dichtel
Cc: Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, kernel-janitors
We need to move the derefernce after the IS_ERR() check.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index a4d9126..24549b4 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -854,14 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
rtnl_lock();
itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
- /* FB netdevice is special: we have one, and only one per netns.
- * Allowing to move it to another netns is clearly unsafe.
- */
- itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
rtnl_unlock();
if (IS_ERR(itn->fb_tunnel_dev))
return PTR_ERR(itn->fb_tunnel_dev);
+ /* FB netdevice is special: we have one, and only one per netns.
+ * Allowing to move it to another netns is clearly unsafe.
+ */
+ itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
return 0;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 7:05 [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net() Dan Carpenter
@ 2013-08-19 7:33 ` Nicolas Dichtel
2013-08-19 12:58 ` Eric Dumazet
2013-08-20 22:08 ` [patch -next] ipip: dereferencing an ERR_PTR " David Miller
2 siblings, 0 replies; 9+ messages in thread
From: Nicolas Dichtel @ 2013-08-19 7:33 UTC (permalink / raw)
To: Dan Carpenter
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, kernel-janitors
Le 19/08/2013 09:05, Dan Carpenter a écrit :
> We need to move the derefernce after the IS_ERR() check.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 7:05 [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net() Dan Carpenter
2013-08-19 7:33 ` Nicolas Dichtel
@ 2013-08-19 12:58 ` Eric Dumazet
2013-08-19 13:49 ` Dan Carpenter
2013-08-19 22:23 ` [patch -next v2] " Dan Carpenter
2013-08-20 22:08 ` [patch -next] ipip: dereferencing an ERR_PTR " David Miller
2 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2013-08-19 12:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: David S. Miller, Nicolas Dichtel, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, kernel-janitors
On Mon, 2013-08-19 at 10:05 +0300, Dan Carpenter wrote:
> We need to move the derefernce after the IS_ERR() check.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index a4d9126..24549b4 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -854,14 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
>
> rtnl_lock();
> itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
> - /* FB netdevice is special: we have one, and only one per netns.
> - * Allowing to move it to another netns is clearly unsafe.
> - */
> - itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
> rtnl_unlock();
>
> if (IS_ERR(itn->fb_tunnel_dev))
> return PTR_ERR(itn->fb_tunnel_dev);
> + /* FB netdevice is special: we have one, and only one per netns.
> + * Allowing to move it to another netns is clearly unsafe.
> + */
> + itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
>
> return 0;
> }
> --
Please add to the changelog :
Bug was added in commit 6c742e714d8c2 ("ipip: add x-netns support")
I do not think this fix is safe.
"dev->features |= some_flag;" should be protected by rtnl
So I suggest using instead :
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index a4d9126..830de3f 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -857,13 +857,11 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
/* FB netdevice is special: we have one, and only one per netns.
* Allowing to move it to another netns is clearly unsafe.
*/
- itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+ if (!IS_ERR(itn->fb_tunnel_dev))
+ itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
rtnl_unlock();
- if (IS_ERR(itn->fb_tunnel_dev))
- return PTR_ERR(itn->fb_tunnel_dev);
-
- return 0;
+ return PTR_RET(itn->fb_tunnel_dev);
}
EXPORT_SYMBOL_GPL(ip_tunnel_init_net);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 12:58 ` Eric Dumazet
@ 2013-08-19 13:49 ` Dan Carpenter
2013-08-19 22:23 ` [patch -next v2] " Dan Carpenter
1 sibling, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2013-08-19 13:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Nicolas Dichtel, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, kernel-janitors
Thanks for the review. I will send a v2 patch shortly.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch -next v2] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 12:58 ` Eric Dumazet
2013-08-19 13:49 ` Dan Carpenter
@ 2013-08-19 22:23 ` Dan Carpenter
2013-08-21 6:33 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2013-08-19 22:23 UTC (permalink / raw)
To: David S. Miller, Nicolas Dichtel
Cc: Alexey Kuznetsov, Eric Dumazet, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, kernel-janitors
The __ip_tunnel_create() function returns an ERR_PTR on error so we need
to check for that before dereferencing. This bug was added in commit
6c742e714d8c2 ("ipip: add x-netns support").
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Eric Dumazet said v1 patch looked racy and suggested the v2 fix.
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index a4d9126..830de3f 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -857,13 +857,11 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
/* FB netdevice is special: we have one, and only one per netns.
* Allowing to move it to another netns is clearly unsafe.
*/
- itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+ if (!IS_ERR(itn->fb_tunnel_dev))
+ itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
rtnl_unlock();
- if (IS_ERR(itn->fb_tunnel_dev))
- return PTR_ERR(itn->fb_tunnel_dev);
-
- return 0;
+ return PTR_RET(itn->fb_tunnel_dev);
}
EXPORT_SYMBOL_GPL(ip_tunnel_init_net);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 7:05 [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net() Dan Carpenter
2013-08-19 7:33 ` Nicolas Dichtel
2013-08-19 12:58 ` Eric Dumazet
@ 2013-08-20 22:08 ` David Miller
2 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2013-08-20 22:08 UTC (permalink / raw)
To: dan.carpenter
Cc: nicolas.dichtel, kuznet, jmorris, yoshfuji, kaber, netdev,
kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 19 Aug 2013 10:05:10 +0300
> We need to move the derefernce after the IS_ERR() check.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch -next v2] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()
2013-08-19 22:23 ` [patch -next v2] " Dan Carpenter
@ 2013-08-21 6:33 ` David Miller
2013-08-23 8:15 ` [patch -next] ipip: potential race " Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2013-08-21 6:33 UTC (permalink / raw)
To: dan.carpenter
Cc: nicolas.dichtel, kuznet, eric.dumazet, jmorris, yoshfuji, kaber,
netdev, kernel-janitors
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 20 Aug 2013 01:23:07 +0300
> The __ip_tunnel_create() function returns an ERR_PTR on error so we need
> to check for that before dereferencing. This bug was added in commit
> 6c742e714d8c2 ("ipip: add x-netns support").
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Eric Dumazet said v1 patch looked racy and suggested the v2 fix.
>
This doesn't apply cleanly, you'll need to respin it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch -next] ipip: potential race in ip_tunnel_init_net()
2013-08-21 6:33 ` David Miller
@ 2013-08-23 8:15 ` Dan Carpenter
2013-08-25 22:40 ` David Miller
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2013-08-23 8:15 UTC (permalink / raw)
To: David S. Miller
Cc: Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, kernel-janitors, Eric Dumazet
Eric Dumazet says that my previous fix for an ERR_PTR dereference
(ea857f28ab 'ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()')
could be racy and suggests the following fix instead.
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 24549b4..830de3f 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -854,16 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
rtnl_lock();
itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
- rtnl_unlock();
-
- if (IS_ERR(itn->fb_tunnel_dev))
- return PTR_ERR(itn->fb_tunnel_dev);
/* FB netdevice is special: we have one, and only one per netns.
* Allowing to move it to another netns is clearly unsafe.
*/
- itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+ if (!IS_ERR(itn->fb_tunnel_dev))
+ itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+ rtnl_unlock();
- return 0;
+ return PTR_RET(itn->fb_tunnel_dev);
}
EXPORT_SYMBOL_GPL(ip_tunnel_init_net);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch -next] ipip: potential race in ip_tunnel_init_net()
2013-08-23 8:15 ` [patch -next] ipip: potential race " Dan Carpenter
@ 2013-08-25 22:40 ` David Miller
0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2013-08-25 22:40 UTC (permalink / raw)
To: dan.carpenter
Cc: kuznet, jmorris, yoshfuji, kaber, netdev, kernel-janitors, eric.dumazet
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Fri, 23 Aug 2013 11:15:37 +0300
> Eric Dumazet says that my previous fix for an ERR_PTR dereference
> (ea857f28ab 'ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()')
> could be racy and suggests the following fix instead.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thanks Dan.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-08-25 22:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-19 7:05 [patch -next] ipip: dereferencing an ERR_PTR in ip_tunnel_init_net() Dan Carpenter
2013-08-19 7:33 ` Nicolas Dichtel
2013-08-19 12:58 ` Eric Dumazet
2013-08-19 13:49 ` Dan Carpenter
2013-08-19 22:23 ` [patch -next v2] " Dan Carpenter
2013-08-21 6:33 ` David Miller
2013-08-23 8:15 ` [patch -next] ipip: potential race " Dan Carpenter
2013-08-25 22:40 ` David Miller
2013-08-20 22:08 ` [patch -next] ipip: dereferencing an ERR_PTR " David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).