* Re: [PATCH] netfilter: check if the socket netns is correct. @ 2018-09-27 20:46 Guenter Roeck 2018-09-27 22:58 ` Flavio Leitner 0 siblings, 1 reply; 4+ messages in thread From: Guenter Roeck @ 2018-09-27 20:46 UTC (permalink / raw) To: Flavio Leitner Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev, linux-kernel Hi Flavio, On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote: > Netfilter assumes that if the socket is present in the skb, then > it can be used because that reference is cleaned up while the skb > is crossing netns. > > We want to change that to preserve the socket reference in a future > patch, so this is a preparation updating netfilter to check if the > socket netns matches before use it. > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > Acked-by: Florian Westphal <fw@strlen.de> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- ... > --- a/net/netfilter/xt_socket.c > +++ b/net/netfilter/xt_socket.c > @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, > struct sk_buff *pskb = (struct sk_buff *)skb; > struct sock *sk = skb->sk; > > + if (!net_eq(xt_net(par), sock_net(sk))) > + sk = NULL; > + I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes for me in read_pnet() because sk is NULL. > if (!sk) > sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par)); The old code seems to suggest that sk == NULL was possible. I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I can not guarantee that this really an upstream problem. The change seems odd, though. Are you sure that it is not (or, rather, no longer) necessary to check if sk == NULL before dereferencing it in sock_net() ? > + > if (sk) { > bool wildcard; > bool transparent = true; > @@ -113,8 +117,12 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par) > struct sk_buff *pskb = (struct sk_buff *)skb; > struct sock *sk = skb->sk; > > + if (!net_eq(xt_net(par), sock_net(sk))) > + sk = NULL; > + Same here. > if (!sk) > sk = nf_sk_lookup_slow_v6(xt_net(par), skb, xt_in(par)); > + > if (sk) { > bool wildcard; > bool transparent = true; Thanks, Guenter ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] netfilter: check if the socket netns is correct. 2018-09-27 20:46 [PATCH] netfilter: check if the socket netns is correct Guenter Roeck @ 2018-09-27 22:58 ` Flavio Leitner 2018-09-27 23:08 ` Guenter Roeck 2018-09-28 8:33 ` Pablo Neira Ayuso 0 siblings, 2 replies; 4+ messages in thread From: Flavio Leitner @ 2018-09-27 22:58 UTC (permalink / raw) To: Guenter Roeck Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev, linux-kernel On Thu, Sep 27, 2018 at 01:46:29PM -0700, Guenter Roeck wrote: > Hi Flavio, > > On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote: > > Netfilter assumes that if the socket is present in the skb, then > > it can be used because that reference is cleaned up while the skb > > is crossing netns. > > > > We want to change that to preserve the socket reference in a future > > patch, so this is a preparation updating netfilter to check if the > > socket netns matches before use it. > > > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > > Acked-by: Florian Westphal <fw@strlen.de> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > --- > ... > > --- a/net/netfilter/xt_socket.c > > +++ b/net/netfilter/xt_socket.c > > @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, > > struct sk_buff *pskb = (struct sk_buff *)skb; > > struct sock *sk = skb->sk; > > > > + if (!net_eq(xt_net(par), sock_net(sk))) > > + sk = NULL; > > + > > I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes > for me in read_pnet() because sk is NULL. > > > if (!sk) > > sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par)); > > The old code seems to suggest that sk == NULL was possible. > > I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I > can not guarantee that this really an upstream problem. The change seems > odd, though. Are you sure that it is not (or, rather, no longer) necessary > to check if sk == NULL before dereferencing it in sock_net() ? Oops, it is necessary but if it's not and the netns doesn't match, we need do the lookup. So, could you check if this fixes the problem for you? From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001 From: Flavio Leitner <fbl@redhat.com> Date: Thu, 27 Sep 2018 19:36:28 -0300 Subject: [PATCH] xt_socket: check sk before checking for netns. Only check for the network namespace if the socket is available. Fixes: f564650106a6 ("netfilter: check if the socket netns is correct.") Reported-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Flavio Leitner <fbl@redhat.com> --- net/netfilter/xt_socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c index 0472f3472842..ada144e5645b 100644 --- a/net/netfilter/xt_socket.c +++ b/net/netfilter/xt_socket.c @@ -56,7 +56,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, struct sk_buff *pskb = (struct sk_buff *)skb; struct sock *sk = skb->sk; - if (!net_eq(xt_net(par), sock_net(sk))) + if (sk && !net_eq(xt_net(par), sock_net(sk))) sk = NULL; if (!sk) @@ -117,7 +117,7 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par) struct sk_buff *pskb = (struct sk_buff *)skb; struct sock *sk = skb->sk; - if (!net_eq(xt_net(par), sock_net(sk))) + if (sk && !net_eq(xt_net(par), sock_net(sk))) sk = NULL; if (!sk) -- 2.14.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] netfilter: check if the socket netns is correct. 2018-09-27 22:58 ` Flavio Leitner @ 2018-09-27 23:08 ` Guenter Roeck 2018-09-28 8:33 ` Pablo Neira Ayuso 1 sibling, 0 replies; 4+ messages in thread From: Guenter Roeck @ 2018-09-27 23:08 UTC (permalink / raw) To: Flavio Leitner Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev, linux-kernel On Thu, Sep 27, 2018 at 07:58:24PM -0300, Flavio Leitner wrote: > On Thu, Sep 27, 2018 at 01:46:29PM -0700, Guenter Roeck wrote: > > Hi Flavio, > > > > On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote: > > > Netfilter assumes that if the socket is present in the skb, then > > > it can be used because that reference is cleaned up while the skb > > > is crossing netns. > > > > > > We want to change that to preserve the socket reference in a future > > > patch, so this is a preparation updating netfilter to check if the > > > socket netns matches before use it. > > > > > > Signed-off-by: Flavio Leitner <fbl@redhat.com> > > > Acked-by: Florian Westphal <fw@strlen.de> > > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > --- > > ... > > > --- a/net/netfilter/xt_socket.c > > > +++ b/net/netfilter/xt_socket.c > > > @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, > > > struct sk_buff *pskb = (struct sk_buff *)skb; > > > struct sock *sk = skb->sk; > > > > > > + if (!net_eq(xt_net(par), sock_net(sk))) > > > + sk = NULL; > > > + > > > > I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes > > for me in read_pnet() because sk is NULL. > > > > > if (!sk) > > > sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par)); > > > > The old code seems to suggest that sk == NULL was possible. > > > > I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I > > can not guarantee that this really an upstream problem. The change seems > > odd, though. Are you sure that it is not (or, rather, no longer) necessary > > to check if sk == NULL before dereferencing it in sock_net() ? > > Oops, it is necessary but if it's not and the netns doesn't match, we need > do the lookup. So, could you check if this fixes the problem for you? > > From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001 > From: Flavio Leitner <fbl@redhat.com> > Date: Thu, 27 Sep 2018 19:36:28 -0300 > Subject: [PATCH] xt_socket: check sk before checking for netns. > > Only check for the network namespace if the socket is available. > > Fixes: f564650106a6 ("netfilter: check if the socket netns is correct.") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Flavio Leitner <fbl@redhat.com> This fixes the problem for me. Tested-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter > --- > net/netfilter/xt_socket.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c > index 0472f3472842..ada144e5645b 100644 > --- a/net/netfilter/xt_socket.c > +++ b/net/netfilter/xt_socket.c > @@ -56,7 +56,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par, > struct sk_buff *pskb = (struct sk_buff *)skb; > struct sock *sk = skb->sk; > > - if (!net_eq(xt_net(par), sock_net(sk))) > + if (sk && !net_eq(xt_net(par), sock_net(sk))) > sk = NULL; > > if (!sk) > @@ -117,7 +117,7 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par) > struct sk_buff *pskb = (struct sk_buff *)skb; > struct sock *sk = skb->sk; > > - if (!net_eq(xt_net(par), sock_net(sk))) > + if (sk && !net_eq(xt_net(par), sock_net(sk))) > sk = NULL; > > if (!sk) > -- > 2.14.4 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] netfilter: check if the socket netns is correct. 2018-09-27 22:58 ` Flavio Leitner 2018-09-27 23:08 ` Guenter Roeck @ 2018-09-28 8:33 ` Pablo Neira Ayuso 1 sibling, 0 replies; 4+ messages in thread From: Pablo Neira Ayuso @ 2018-09-28 8:33 UTC (permalink / raw) To: Flavio Leitner Cc: Guenter Roeck, Jozsef Kadlecsik, Florian Westphal, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev, linux-kernel On Thu, Sep 27, 2018 at 07:58:24PM -0300, Flavio Leitner wrote: [...] > From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001 > From: Flavio Leitner <fbl@redhat.com> > Date: Thu, 27 Sep 2018 19:36:28 -0300 > Subject: [PATCH] xt_socket: check sk before checking for netns. > > Only check for the network namespace if the socket is available. Applied. Please, Cc netfilter-devel@vger.kernel.org next time for your netfilter patches. Thanks. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-28 8:33 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-27 20:46 [PATCH] netfilter: check if the socket netns is correct Guenter Roeck 2018-09-27 22:58 ` Flavio Leitner 2018-09-27 23:08 ` Guenter Roeck 2018-09-28 8:33 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox