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

[David Miller]

From: Vegard Nossum 
Date: Sat, 23 Jul 2016 09:42:35 +0200

> I was seeing a lot of these:
 ...
> 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 
> Cc: Herbert Xu 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Vegard Nossum 

Applied and queued up for -stable, thanks.

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

[Marcelo Ricardo Leitner]

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:[] 
> rhashtable_walk_start+0x46/0x150
> 
>  [] preempt_count_add+0x1fb/0x280
>  [] _raw_spin_lock+0x12/0x40
>  [] console_unlock+0x2f7/0x930
>  [] vprintk_emit+0x2fb/0x520
>  [] vprintk_default+0x1a/0x20
>  [] printk+0x94/0xb0
>  [] print_stack_trace+0xe0/0x170
>  [] ___might_sleep+0x3be/0x460
>  [] __might_sleep+0x90/0x1a0
>  [] kmem_cache_alloc+0x153/0x1e0
>  [] rhashtable_walk_init+0xfe/0x2d0
>  [] sctp_transport_walk_start+0x1e/0x60
>  [] sctp_transport_seq_start+0x4d/0x150
>  [] seq_read+0x27b/0x1180
>  [] proc_reg_read+0xbc/0x180
>  [] __vfs_read+0xdb/0x610
>  [] vfs_read+0xea/0x2d0
>  [] SyS_pread64+0x11b/0x150
>  [] do_syscall_64+0x19c/0x410
>  [] return_from_SYSCALL_64+0x0/0x6a
>  [] 0x
> 
> 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 
> Cc: Herbert Xu 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Vegard Nossum 
> ---
>  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 

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

[PATCH] net/sctp: terminate rhashtable walk correctly

[Vegard Nossum]

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:[] rhashtable_walk_start+0x46/0x150

 [] preempt_count_add+0x1fb/0x280
 [] _raw_spin_lock+0x12/0x40
 [] console_unlock+0x2f7/0x930
 [] vprintk_emit+0x2fb/0x520
 [] vprintk_default+0x1a/0x20
 [] printk+0x94/0xb0
 [] print_stack_trace+0xe0/0x170
 [] ___might_sleep+0x3be/0x460
 [] __might_sleep+0x90/0x1a0
 [] kmem_cache_alloc+0x153/0x1e0
 [] rhashtable_walk_init+0xfe/0x2d0
 [] sctp_transport_walk_start+0x1e/0x60
 [] sctp_transport_seq_start+0x4d/0x150
 [] seq_read+0x27b/0x1180
 [] proc_reg_read+0xbc/0x180
 [] __vfs_read+0xdb/0x610
 [] vfs_read+0xea/0x2d0
 [] SyS_pread64+0x11b/0x150
 [] do_syscall_64+0x19c/0x410
 [] return_from_SYSCALL_64+0x0/0x6a
 [] 0x

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 
Cc: Herbert Xu 
Cc: sta...@vger.kernel.org
Signed-off-by: Vegard Nossum 
---
 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);
return err;
}
-- 
1.9.1