Re: [PATCH net-next] net: proper locking in skb_update_prio()

Neil Horman
2011-11-30T11:41:40Z

On Wed, Nov 30, 2011 at 12:24:38AM +0100, Eric Dumazet wrote:
> Le mercredi 30 novembre 2011 à 00:11 +0100, Eric Dumazet a écrit :
>
> > Some changes are needed now rcu_read_lock_bh() doesnt imply
> > rcu_read_lock().
> >
> > For example, recently added skb_update_prio() is buggy, since it uses
> > rcu_dereference() while its caller, dev_queue_xmit() called
> > rcu_read_lock_bh()
> >
> >
>
> [PATCH net-next] net: proper locking in skb_update_prio()
>
> We must use rcu_read_lock() in skb_update_prio(), since dev_queue_xmit()
> uses rcu_read_lock_bh()
>
> [ 15.441620] [ INFO: suspicious RCU usage. ]
> [ 15.441622] -------------------------------
> [ 15.441624] net/core/dev.c:2476 suspicious rcu_dereference_check() usage!
> [ 15.441625]
> [ 15.441626] other info that might help us debug this:
> [ 15.441626]
> [ 15.441628]
> [ 15.441628] rcu_scheduler_active = 1, debug_locks = 1
> [ 15.441630] 1 lock held by arping/4373:
> [ 15.441632] #0: (rcu_read_lock_bh){......}, at: [<c13049b0>] dev_queue_xmit+0x0/0xa90
> [ 15.441641]
> [ 15.441642] stack backtrace:
> [ 15.441644] Pid: 4373, comm: arping Not tainted 3.2.0-rc2-12727-gd69d22a-dirty #1261
> [ 15.441646] Call Trace:
> [ 15.441651] [<c13bae42>] ? printk+0x18/0x1e
> [ 15.441656] [<c107f1aa>] lockdep_rcu_suspicious+0xaa/0xc0
> [ 15.441658] [<c130507a>] dev_queue_xmit+0x6ca/0xa90
> [ 15.441661] [<c13049b0>] ? dev_hard_start_xmit+0x810/0x810
> [ 15.441665] [<c131cb84>] ? eth_header+0x24/0xb0
> [ 15.441668] [<c139c4f8>] packet_sendmsg+0x978/0x9d0
> [ 15.441671] [<c131cb60>] ? eth_rebuild_header+0x80/0x80
> [ 15.441675] [<c12f3173>] ? sock_update_netprioidx+0xa3/0x110
> [ 15.441678] [<c12ee93e>] sock_sendmsg+0xce/0x100
> [ 15.441682] [<c10e354e>] ? might_fault+0x2e/0x80
> [ 15.441684] [<c10e354e>] ? might_fault+0x2e/0x80
> [ 15.441687] [<c10e3594>] ? might_fault+0x74/0x80
> [ 15.441691] [<c11ce55f>] ? _copy_from_user+0x3f/0x60
> [ 15.441693] [<c12f03e2>] sys_sendto+0xb2/0xe0
> [ 15.441696] [<c108288b>] ? lock_release_non_nested+0x8b/0x300
> [ 15.441699] [<c10e354e>] ? might_fault+0x2e/0x80
> [ 15.441701] [<c10e354e>] ? might_fault+0x2e/0x80
> [ 15.441704] [<c12f0cd0>] sys_socketcall+0x1a0/0x280
> [ 15.441708] [<c13bfc90>] sysenter_do_call+0x12/0x36
>
> Signed-off-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> ---
> net/core/dev.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 91a5991..903fd9d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2473,10 +2473,15 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> #if IS_ENABLED(CONFIG_NETPRIO_CGROUP)
> static void skb_update_prio(struct sk_buff *skb)
> {
> - struct netprio_map *map = rcu_dereference(skb->dev->priomap);
> + if (!skb->priority && skb->sk) {
> + struct netprio_map *map;
>
> - if ((!skb->priority) && (skb->sk) && map)
> - skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
> + rcu_read_lock();
> + map = rcu_dereference(skb->dev->priomap);
> + if (map)
> + skb->priority = map->priomap[skb->sk->sk_cgrp_prioidx];
> + rcu_read_unlock();
> + }
> }
> #else
> #define skb_update_prio(skb)
>
>
>
Ack, thanks. I thought the rcu_read_lock in dev_queue_xmit was sufficient.

Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>