Re: [PATCH v3] nbd: Fix NULL pointer in flush_workqueue

[Markus Elfring <Markus.Elfring@web.de>]

> v3: Do not use unkock and add put_nbd.

* How do you think about to avoid a typo here?
* Can such a wording become clearer another bit?


…
> +++ b/drivers/block/nbd.c
> @@ -2011,19 +2011,23 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
>  		       index);
>  		return -EINVAL;
>  	}
> -	if (!refcount_inc_not_zero(&nbd->refs)) {
> +	mutex_lock(&nbd->config_lock);
> +	if (!refcount_inc_not_zero(&nbd->refs) || !nbd->recv_workq) {
> +		mutex_unlock(&nbd->config_lock);
>  		mutex_unlock(&nbd_index_mutex);
>  		printk(KERN_ERR "nbd: device at index %d is going down\n",
>  		       index);
>  		return -EINVAL;
>  	}

Can a bit more exception handling code be shared for these if branches?

 	if (!nbd) {
-		mutex_unlock(&nbd_index_mutex);
 		printk(KERN_ERR "nbd: couldn't find device at index %d\n",
 		       index);
-		return -EINVAL;
+		goto unlock_index;
	}
+	if (!refcount_inc_not_zero(&nbd->refs) || !nbd->recv_workq) {
+		mutex_unlock(&nbd->config_lock);
-		mutex_unlock(&nbd_index_mutex);
 		printk(KERN_ERR "nbd: device at index %d is going down\n",
 		       index);
+unlock_index:
+		mutex_unlock(&nbd_index_mutex);
 		return -EINVAL;
 	}


>  	mutex_unlock(&nbd_index_mutex);
> -	if (!refcount_inc_not_zero(&nbd->config_refs)) {
> -		nbd_put(nbd);
> -		return 0;
> -	}
> +	if (!refcount_inc_not_zero(&nbd->config_refs))
> +		goto put_nbd;
>  	nbd_disconnect_and_put(nbd);
>  	nbd_config_put(nbd);
> +	goto put_nbd;

Please delete the extra jump which I find questionable at this place.


> +
> +put_nbd:
>  	nbd_put(nbd);
>  	return 0;
>  }

Please move the part for such a source code adjustment to a separate patch.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=88bb507a74ea7d75fa49edd421eaa710a7d80598#n76


Regards,
Markus