Re: [PATCH] IB/hfi1,IB/qib: Fix qp_stats sleep with rcu read lock held

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5494 bytes --]

On Tue, Aug 09, 2016 at 11:16:26AM -0400, ira.weiny@intel.com wrote:
> From: Mike Marciniszyn <mike.marciniszyn@intel.com>
> 
> The qp init function does a kzalloc() while holding the RCU
> lock that encounters the following warning with a debug kernel
> when a cat of the qp_stats is done:
> 
> [  231.723948] rcu_scheduler_active = 1, debug_locks = 0
> [  231.731939] 3 locks held by cat/11355:
> [  231.736492]  #0:  (debugfs_srcu){......}, at: [<ffffffff813001a5>] debugfs_use_file_start+0x5/0x90
> [  231.746955]  #1:  (&p->lock){+.+.+.}, at: [<ffffffff81289a6c>] seq_read+0x4c/0x3c0
> [  231.755873]  #2:  (rcu_read_lock){......}, at: [<ffffffffa0a0c535>] _qp_stats_seq_start+0x5/0xd0 [hfi1]
> [  231.766862]
> 
> The init functions do an implicit next which requires the rcu read lock
> before the kzalloc().
> 
> Fix for both drivers is to change the scope of the init function to only
> do the allocation and the initialization of the just allocated iter.
> 
> The implict next is moved back into the respective start functions to fix
> the issue.
> 
> 
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> CC: <stable@vger.kernel.org> # 4.6.x-
> ---
>  drivers/infiniband/hw/hfi1/debugfs.c    | 17 ++++++++++++-----
>  drivers/infiniband/hw/hfi1/qp.c         |  4 ----
>  drivers/infiniband/hw/qib/qib_debugfs.c | 16 ++++++++++++----
>  drivers/infiniband/hw/qib/qib_qp.c      |  4 ----
>  4 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
> index dbab9d9cc288..c35bef8dd5aa 100644
> --- a/drivers/infiniband/hw/hfi1/debugfs.c
> +++ b/drivers/infiniband/hw/hfi1/debugfs.c
> @@ -223,28 +223,35 @@ DEBUGFS_SEQ_FILE_OPEN(ctx_stats)
>  DEBUGFS_FILE_OPS(ctx_stats);
>  
>  static void *_qp_stats_seq_start(struct seq_file *s, loff_t *pos)
> -__acquires(RCU)
> +	__acquires(RCU)
>  {
>  	struct qp_iter *iter;
>  	loff_t n = *pos;
>  
> -	rcu_read_lock();
>  	iter = qp_iter_init(s->private);
> +
> +	/* stop calls rcu_read_unlock */
> +	rcu_read_lock();

IMHO, it should be placed after your if(!iter) check below.

> +
>  	if (!iter)
>  		return NULL;
>  
> -	while (n--) {
> +	if (qp_iter_next(iter)) {
> +		kfree(iter);
> +		return NULL;
> +	}
> +	while (n--)
>  		if (qp_iter_next(iter)) {
>  			kfree(iter);
>  			return NULL;
>  		}

It looks like you forgot to remove the lines above.

> -	}
>  
>  	return iter;
>  }
>  
>  static void *_qp_stats_seq_next(struct seq_file *s, void *iter_ptr,
>  				loff_t *pos)
> +	__must_hold(RCU)
>  {
>  	struct qp_iter *iter = iter_ptr;
>  
> @@ -259,7 +266,7 @@ static void *_qp_stats_seq_next(struct seq_file *s, void *iter_ptr,
>  }
>  
>  static void _qp_stats_seq_stop(struct seq_file *s, void *iter_ptr)
> -__releases(RCU)
> +	__releases(RCU)
>  {
>  	rcu_read_unlock();
>  }
> diff --git a/drivers/infiniband/hw/hfi1/qp.c b/drivers/infiniband/hw/hfi1/qp.c
> index a5aa3517e7d5..4e4d8317c281 100644
> --- a/drivers/infiniband/hw/hfi1/qp.c
> +++ b/drivers/infiniband/hw/hfi1/qp.c
> @@ -656,10 +656,6 @@ struct qp_iter *qp_iter_init(struct hfi1_ibdev *dev)
>  
>  	iter->dev = dev;
>  	iter->specials = dev->rdi.ibdev.phys_port_cnt * 2;
> -	if (qp_iter_next(iter)) {
> -		kfree(iter);
> -		return NULL;
> -	}
>  
>  	return iter;
>  }
> diff --git a/drivers/infiniband/hw/qib/qib_debugfs.c b/drivers/infiniband/hw/qib/qib_debugfs.c
> index 5e75b43c596b..07059c08c170 100644
> --- a/drivers/infiniband/hw/qib/qib_debugfs.c
> +++ b/drivers/infiniband/hw/qib/qib_debugfs.c
> @@ -189,27 +189,34 @@ static int _ctx_stats_seq_show(struct seq_file *s, void *v)
>  DEBUGFS_FILE(ctx_stats)
>  
>  static void *_qp_stats_seq_start(struct seq_file *s, loff_t *pos)
> +	__acquires(RCU)
>  {
>  	struct qib_qp_iter *iter;
>  	loff_t n = *pos;
>  
> -	rcu_read_lock();
>  	iter = qib_qp_iter_init(s->private);
> +
> +	/* stop calls rcu_read_unlock */
> +	rcu_read_lock();
> +

The same

>  	if (!iter)
>  		return NULL;
>  
> -	while (n--) {
> +	if (qib_qp_iter_next(iter)) {
> +		kfree(iter);
> +		return NULL;
> +	}
> +	while (n--)
>  		if (qib_qp_iter_next(iter)) {
>  			kfree(iter);
>  			return NULL;
>  		}
> -	}
> -

The same

>  	return iter;
>  }
>  
>  static void *_qp_stats_seq_next(struct seq_file *s, void *iter_ptr,
>  				   loff_t *pos)
> +	__must_hold(RCU)
>  {
>  	struct qib_qp_iter *iter = iter_ptr;
>  
> @@ -224,6 +231,7 @@ static void *_qp_stats_seq_next(struct seq_file *s, void *iter_ptr,
>  }
>  
>  static void _qp_stats_seq_stop(struct seq_file *s, void *iter_ptr)
> +	__releases(RCU)
>  {
>  	rcu_read_unlock();
>  }
> diff --git a/drivers/infiniband/hw/qib/qib_qp.c b/drivers/infiniband/hw/qib/qib_qp.c
> index 9cc0aae1d781..f9b8cd2354d1 100644
> --- a/drivers/infiniband/hw/qib/qib_qp.c
> +++ b/drivers/infiniband/hw/qib/qib_qp.c
> @@ -573,10 +573,6 @@ struct qib_qp_iter *qib_qp_iter_init(struct qib_ibdev *dev)
>  		return NULL;
>  
>  	iter->dev = dev;
> -	if (qib_qp_iter_next(iter)) {
> -		kfree(iter);
> -		return NULL;
> -	}
>  
>  	return iter;
>  }
> -- 
> 1.8.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]