netdev - Re: [PATCH] net/sctp: terminate rhashtable walk correctly

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]

Message-ID: <20160723132600.GF9950@localhost.localdomain>
Date:	Sat, 23 Jul 2016 10:26:00 -0300
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Vegard Nossum <vegard.nossum@...cle.com>
Cc:	Vlad Yasevich <vyasevich@...il.com>,
	Neil Horman <nhorman@...driver.com>,
	linux-sctp@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Xin Long <lucien.xin@...il.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	stable@...r.kernel.org
Subject: Re: [PATCH] net/sctp: terminate rhashtable walk correctly

On Sat, Jul 23, 2016 at 09:42:35AM +0200, Vegard Nossum wrote:
> I was seeing a lot of these:
> 
>     BUG: sleeping function called from invalid context at mm/slab.h:388
>     in_atomic(): 0, irqs_disabled(): 0, pid: 14971, name: trinity-c2
>     Preemption disabled at:[<ffffffff819bcd46>] rhashtable_walk_start+0x46/0x150
> 
>      [<ffffffff81149abb>] preempt_count_add+0x1fb/0x280
>      [<ffffffff83295722>] _raw_spin_lock+0x12/0x40
>      [<ffffffff811aac87>] console_unlock+0x2f7/0x930
>      [<ffffffff811ab5bb>] vprintk_emit+0x2fb/0x520
>      [<ffffffff811aba6a>] vprintk_default+0x1a/0x20
>      [<ffffffff812c171a>] printk+0x94/0xb0
>      [<ffffffff811d6ed0>] print_stack_trace+0xe0/0x170
>      [<ffffffff8115835e>] ___might_sleep+0x3be/0x460
>      [<ffffffff81158490>] __might_sleep+0x90/0x1a0
>      [<ffffffff8139b823>] kmem_cache_alloc+0x153/0x1e0
>      [<ffffffff819bca1e>] rhashtable_walk_init+0xfe/0x2d0
>      [<ffffffff82ec64de>] sctp_transport_walk_start+0x1e/0x60
>      [<ffffffff82edd8ad>] sctp_transport_seq_start+0x4d/0x150
>      [<ffffffff8143a82b>] seq_read+0x27b/0x1180
>      [<ffffffff814f97fc>] proc_reg_read+0xbc/0x180
>      [<ffffffff813d471b>] __vfs_read+0xdb/0x610
>      [<ffffffff813d4d3a>] vfs_read+0xea/0x2d0
>      [<ffffffff813d615b>] SyS_pread64+0x11b/0x150
>      [<ffffffff8100334c>] do_syscall_64+0x19c/0x410
>      [<ffffffff832960a5>] return_from_SYSCALL_64+0x0/0x6a
>      [<ffffffffffffffff>] 0xffffffffffffffff
> 
> Apparently we always need to call rhashtable_walk_stop(), even when
> rhashtable_walk_start() fails:
> 
>  * rhashtable_walk_start - Start a hash table walk
>  * @iter:       Hash table iterator
>  *
>  * Start a hash table walk.  Note that we take the RCU lock in all
>  * cases including when we return an error.  So you must always call
>  * rhashtable_walk_stop to clean up.
> 
> otherwise we never call rcu_read_unlock() and we get the splat above.
> 
> See-also: 53fa1036 ("sctp: fix some rhashtable functions using in sctp proc/diag")
> See-also: f2dba9c6 ("rhashtable: Introduce rhashtable_walk_*")
> Cc: Xin Long <lucien.xin@...il.com>
> Cc: Herbert Xu <herbert@...dor.apana.org.au>
> Cc: stable@...r.kernel.org
> Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
> ---
>  net/sctp/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 67154b8..7f5689a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4301,6 +4301,7 @@ int sctp_transport_walk_start(struct rhashtable_iter *iter)
>  
>  	err = rhashtable_walk_start(iter);
>  	if (err && err != -EAGAIN) {
> +		rhashtable_walk_stop(iter);
>  		rhashtable_walk_exit(iter);

I was doubt now if we shouldn't just call sctp_transport_walk_stop()
instead, but maybe it's good to keep the two apart, error handling from
actual stop.

I think this one deserves a Fixes tag because prior to 53fa1036 it was
okay.

Fixes: 53fa1036 ("sctp: fix some rhashtable functions using in sctp proc/diag")
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

>  		return err;
>  	}
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>