netdev - Re: [PATCH net-next] net: proper locking in skb_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Nov 2011 06:41:40 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Marc Aurele La France <tsi@...berta.ca>,
	Roland Dreier <roland@...nel.org>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org
Subject: Re: [PATCH net-next] net: proper locking in skb_update_prio()

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@...il.com>
> CC: Neil Horman <nhorman@...driver.com>
> ---
>  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@...driver.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html