| Message ID | 492A5F6D.3040708@cosmosbay.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <dada1@cosmosbay.com> Date: Mon, 24 Nov 2008 09:01:49 +0100 > David Miller a écrit : > > From: David Miller <davem@davemloft.net> > > Date: Sun, 23 Nov 2008 17:20:14 -0800 (PST) > > > >> From: Eric Dumazet <dada1@cosmosbay.com> > >> Date: Sun, 23 Nov 2008 04:32:30 +0100 > >> > >>> [PATCH] net: make sock_prot_inuse_add() preempt safe > > ... > >> Eric, you added this bug by starting to use this interface in > >> situations where BH's were not disabled. > >> > >> Ever existing use adhered to that rule. > >> > >> If you therefore want to call this interface in new locations, > >> you have to make sure those locations follow the rule too. > > Here is what I commited to fix this bug. > > net: Make sure BHs are disabled in sock_prot_inuse_add() > > The rule of calling sock_prot_inuse_add() is that BHs must > > be disabled. Some new calls were added where this was not > > true and this tiggers warnings as reported by Ilpo. > > Fix this by adding explicit BH disabling around those call sites. > > Signed-off-by: David S. Miller <davem@davemloft.net> > > --- > > net/netlink/af_netlink.c | 3 +++ > > net/sctp/socket.c | 4 ++++ > > net/unix/af_unix.c | 2 ++ > > 3 files changed, 9 insertions(+), 0 deletions(-) > > I believe some bits are missing > > Ilpo report was about unix_create1() being preemptable for example Thanks for catching that oversight, applied. -- 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, 24 Nov 2008, Eric Dumazet wrote: > David Miller a écrit : > > From: David Miller <davem@davemloft.net> > > Date: Sun, 23 Nov 2008 17:20:14 -0800 (PST) > > > > > From: Eric Dumazet <dada1@cosmosbay.com> > > > Date: Sun, 23 Nov 2008 04:32:30 +0100 > > > > > > > [PATCH] net: make sock_prot_inuse_add() preempt safe > > ... > > > Eric, you added this bug by starting to use this interface in > > > situations where BH's were not disabled. > > > > > > Ever existing use adhered to that rule. > > > > > > If you therefore want to call this interface in new locations, > > > you have to make sure those locations follow the rule too. > > > > Here is what I commited to fix this bug. > > > > net: Make sure BHs are disabled in sock_prot_inuse_add() > > > > The rule of calling sock_prot_inuse_add() is that BHs must > > be disabled. Some new calls were added where this was not > > true and this tiggers warnings as reported by Ilpo. > > > > Fix this by adding explicit BH disabling around those call sites. > > > > Signed-off-by: David S. Miller <davem@davemloft.net> > > --- > > net/netlink/af_netlink.c | 3 +++ > > net/sctp/socket.c | 4 ++++ > > net/unix/af_unix.c | 2 ++ > > 3 files changed, 9 insertions(+), 0 deletions(-) > > I believe some bits are missing > > Ilpo report was about unix_create1() being preemptable for example Yes, basically these two sites I got (during boot, I didn't look much further if I could have gotten them from somewhere else too): $ grep -A1 '[]] sock_prot_inuse_add[+]' dmesg.log | grep "fffff" | grep -v sock_prot_inuse_add | sort | uniq [<ffffffff80514cdd>] unix_create1+0x161/0x176 [<ffffffff805151de>] unix_sock_destructor+0xb6/0xbc
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c index 11fcb87..6a1045d 100644 --- a/net/ipv4/inet_hashtables.c +++ b/net/ipv4/inet_hashtables.c @@ -402,9 +402,9 @@ void inet_unhash(struct sock *sk) spin_lock_bh(lock); done =__sk_nulls_del_node_init_rcu(sk); - spin_unlock_bh(lock); if (done) sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1); + spin_unlock_bh(lock); } EXPORT_SYMBOL_GPL(inet_unhash); diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index b4870a3..5f94db2 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -872,6 +872,7 @@ static int packet_release(struct socket *sock) write_lock_bh(&net->packet.sklist_lock); sk_del_node_init(sk); + sock_prot_inuse_add(net, sk->sk_prot, -1); write_unlock_bh(&net->packet.sklist_lock); /* @@ -910,7 +911,6 @@ static int packet_release(struct socket *sock) skb_queue_purge(&sk->sk_receive_queue); sk_refcnt_debug_release(sk); - sock_prot_inuse_add(net, sk->sk_prot, -1); sock_put(sk); return 0; } @@ -1085,8 +1085,8 @@ static int packet_create(struct net *net, struct socket *sock, int protocol) write_lock_bh(&net->packet.sklist_lock); sk_add_node(sk, &net->packet.sklist); - write_unlock_bh(&net->packet.sklist_lock); sock_prot_inuse_add(net, &packet_proto, 1); + write_unlock_bh(&net->packet.sklist_lock); return(0); out: return err; diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index a45a9f7..3a35a6e 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -615,9 +615,11 @@ static struct sock *unix_create1(struct net *net, struct socket *sock) out: if (sk == NULL) atomic_dec(&unix_nr_socks); - else + else { + local_bh_disable(); sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1); - + local_bh_enable(); + } return sk; }