Patchwork [3/6] netfilter: ctnetlink: use GFP_ATOMIC in all allocations

login
register
mail settings
Submitter Pablo Neira
Date March 6, 2012, 11:22 a.m.
Message ID <1331032975-5303-4-git-send-email-pablo@netfilter.org>
Download mbox | patch
Permalink /patch/144915/
State Awaiting Upstream
Headers show

Comments

Pablo Neira - March 6, 2012, 11:22 a.m.
From: Pablo Neira Ayuso <pablo@netfilter.org>

All ctnetlink operations are invoked inside rcu_read_lock
(see net/netfilter/nfnetlink.c).

Allocations have to be atomic, as RCU requires.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_netlink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Eric Dumazet - March 6, 2012, 12:50 p.m.
On Tue, 2012-03-06 at 12:22 +0100, pablo@netfilter.org wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> All ctnetlink operations are invoked inside rcu_read_lock
> (see net/netfilter/nfnetlink.c).
> 
> Allocations have to be atomic, as RCU requires.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 1068769..867843f 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1002,7 +1002,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
>  	ct = nf_ct_tuplehash_to_ctrack(h);
>  
>  	err = -ENOMEM;
> -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>  	if (skb2 == NULL) {
>  		nf_ct_put(ct);
>  		return -ENOMEM;
> @@ -1865,7 +1865,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
>  	}
>  
>  	err = -ENOMEM;
> -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>  	if (skb2 == NULL) {
>  		nf_ct_expect_put(exp);
>  		goto out;

This cant be right.

Really this must be kept as GFP_KERNEL allocations.

Only if .call_rcu member is used in place of .call rcu_read_lock() is
held instead of nfnl_lock().

You should take a look at all GFP_ATOMIC uses in
net/netfilter/nf_conntrack_netlink.c and check if they can be GFP_KERNEL
instead.



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - March 6, 2012, 2:48 p.m.
On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
> On Tue, 2012-03-06 at 12:22 +0100, pablo@netfilter.org wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > All ctnetlink operations are invoked inside rcu_read_lock
> > (see net/netfilter/nfnetlink.c).
> > 
> > Allocations have to be atomic, as RCU requires.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_conntrack_netlink.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index 1068769..867843f 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -1002,7 +1002,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
> >  	ct = nf_ct_tuplehash_to_ctrack(h);
> >  
> >  	err = -ENOMEM;
> > -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> >  	if (skb2 == NULL) {
> >  		nf_ct_put(ct);
> >  		return -ENOMEM;
> > @@ -1865,7 +1865,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
> >  	}
> >  
> >  	err = -ENOMEM;
> > -	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> >  	if (skb2 == NULL) {
> >  		nf_ct_expect_put(exp);
> >  		goto out;
> 
> This cant be right.
> 
> Really this must be kept as GFP_KERNEL allocations.
> 
> Only if .call_rcu member is used in place of .call rcu_read_lock() is
> held instead of nfnl_lock().

I thought we couldn't sleep while holding rcu_read_lock.

> You should take a look at all GFP_ATOMIC uses in
> net/netfilter/nf_conntrack_netlink.c and check if they can be GFP_KERNEL
> instead.

David, can you take all patches except this one?

I'll have to rebase my tree after this, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - March 6, 2012, 3:09 p.m.
Le mardi 06 mars 2012 à 15:48 +0100, Pablo Neira Ayuso a écrit :
> On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:

> > This cant be right.
> > 
> > Really this must be kept as GFP_KERNEL allocations.
> > 
> > Only if .call_rcu member is used in place of .call rcu_read_lock() is
> > held instead of nfnl_lock().
> 
> I thought we couldn't sleep while holding rcu_read_lock.

True, but as far as I can see we dont hold rcu_read_lock() at this
point, only a mutex.

I added the .call_rcu() mechanism in struct nfnl_callback only for very
specific needs, namely performance improvements in commit 84a797dd0
(netfilter: nfnetlink_queue: provide rcu enabled callbacks)



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - March 6, 2012, 3:35 p.m.
On Tue, Mar 06, 2012 at 07:09:17AM -0800, Eric Dumazet wrote:
> Le mardi 06 mars 2012 à 15:48 +0100, Pablo Neira Ayuso a écrit :
> > On Tue, Mar 06, 2012 at 04:50:21AM -0800, Eric Dumazet wrote:
> 
> > > This cant be right.
> > > 
> > > Really this must be kept as GFP_KERNEL allocations.
> > > 
> > > Only if .call_rcu member is used in place of .call rcu_read_lock() is
> > > held instead of nfnl_lock().
> > 
> > I thought we couldn't sleep while holding rcu_read_lock.
> 
> True, but as far as I can see we dont hold rcu_read_lock() at this
> point, only a mutex.
> 
> I added the .call_rcu() mechanism in struct nfnl_callback only for very
> specific needs, namely performance improvements in commit 84a797dd0
> (netfilter: nfnetlink_queue: provide rcu enabled callbacks)

Sorry, I overlooked that changed, I still thought that we were calling
these under rcu_read_lock.

This patch has to be kept out indeed. Thanks for spotting this Eric.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 6, 2012, 8:20 p.m.
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 6 Mar 2012 15:48:38 +0100

> David, can you take all patches except this one?

That's what I did, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 1068769..867843f 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1002,7 +1002,7 @@  ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
 	ct = nf_ct_tuplehash_to_ctrack(h);
 
 	err = -ENOMEM;
-	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (skb2 == NULL) {
 		nf_ct_put(ct);
 		return -ENOMEM;
@@ -1865,7 +1865,7 @@  ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
 	}
 
 	err = -ENOMEM;
-	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
 	if (skb2 == NULL) {
 		nf_ct_expect_put(exp);
 		goto out;