linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
@ 2020-02-10  7:32 Sun Ke
  2020-02-10 17:05 ` Mike Christie
  0 siblings, 1 reply; 6+ messages in thread
From: Sun Ke @ 2020-02-10  7:32 UTC (permalink / raw)
  To: josef, axboe, mchristi, sunke32; +Cc: linux-block, nbd, linux-kernel

Open /dev/nbdX first, the config_refs will be 1 and
the pointers in nbd_device are still null. Disconnect
/dev/nbdX, then reference a null recv_workq. The
protection by config_refs in nbd_genl_disconnect is useless.

To fix it, just add a check for a non null task_recv in
nbd_genl_disconnect.

Signed-off-by: Sun Ke <sunke32@huawei.com>
---
v1 -> v2:
Add an omitted mutex_unlock.

v2 -> v3:
Add nbd->config_lock, suggested by Josef.
---
 drivers/block/nbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index b4607dd96185..870b3fd0c101 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
 		       index);
 		return -EINVAL;
 	}
+	mutex_lock(&nbd->config_lock);
 	if (!refcount_inc_not_zero(&nbd->refs)) {
+		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;
 	}
+	if (!nbd->recv_workq) {
+		mutex_unlock(&nbd->config_lock);
+		mutex_unlock(&nbd_index_mutex);
+		return -EINVAL;
+	}
+	mutex_unlock(&nbd->config_lock);
 	mutex_unlock(&nbd_index_mutex);
 	if (!refcount_inc_not_zero(&nbd->config_refs)) {
 		nbd_put(nbd);
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
  2020-02-10  7:32 [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect Sun Ke
@ 2020-02-10 17:05 ` Mike Christie
  2020-02-11  4:12   ` sunke (E)
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Christie @ 2020-02-10 17:05 UTC (permalink / raw)
  To: Sun Ke, josef, axboe; +Cc: linux-block, nbd, linux-kernel

On 02/10/2020 01:32 AM, Sun Ke wrote:
> Open /dev/nbdX first, the config_refs will be 1 and
> the pointers in nbd_device are still null. Disconnect
> /dev/nbdX, then reference a null recv_workq. The
> protection by config_refs in nbd_genl_disconnect is useless.
> 
> To fix it, just add a check for a non null task_recv in
> nbd_genl_disconnect.
> 
> Signed-off-by: Sun Ke <sunke32@huawei.com>
> ---
> v1 -> v2:
> Add an omitted mutex_unlock.
> 
> v2 -> v3:
> Add nbd->config_lock, suggested by Josef.
> ---
>  drivers/block/nbd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index b4607dd96185..870b3fd0c101 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
>  		       index);
>  		return -EINVAL;
>  	}
> +	mutex_lock(&nbd->config_lock);
>  	if (!refcount_inc_not_zero(&nbd->refs)) {
> +		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;
>  	}
> +	if (!nbd->recv_workq) {
> +		mutex_unlock(&nbd->config_lock);
> +		mutex_unlock(&nbd_index_mutex);
> +		return -EINVAL;
> +	}
> +	mutex_unlock(&nbd->config_lock);
>  	mutex_unlock(&nbd_index_mutex);
>  	if (!refcount_inc_not_zero(&nbd->config_refs)) {
>  		nbd_put(nbd);
>

With my other patch then we will not need this right? It handles your
case by just being integrated with the existing checks in:

nbd_disconnect_and_put->nbd_clear_sock->sock_shutdown

...

static void sock_shutdown(struct nbd_device *nbd)
{

....

        if (config->num_connections == 0)
                return;


num_connections is zero for your case since we never did a
nbd_genl_disconnect so we would return here.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
  2020-02-10 17:05 ` Mike Christie
@ 2020-02-11  4:12   ` sunke (E)
  2020-02-11 16:39     ` Mike Christie
  0 siblings, 1 reply; 6+ messages in thread
From: sunke (E) @ 2020-02-11  4:12 UTC (permalink / raw)
  To: Mike Christie, josef, axboe; +Cc: linux-block, nbd, linux-kernel



在 2020/2/11 1:05, Mike Christie 写道:
> On 02/10/2020 01:32 AM, Sun Ke wrote:
>> Open /dev/nbdX first, the config_refs will be 1 and
>> the pointers in nbd_device are still null. Disconnect
>> /dev/nbdX, then reference a null recv_workq. The
>> protection by config_refs in nbd_genl_disconnect is useless.
>>
>> To fix it, just add a check for a non null task_recv in
>> nbd_genl_disconnect.
>>
>> Signed-off-by: Sun Ke <sunke32@huawei.com>
>> ---
>> v1 -> v2:
>> Add an omitted mutex_unlock.
>>
>> v2 -> v3:
>> Add nbd->config_lock, suggested by Josef.
>> ---
>>   drivers/block/nbd.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index b4607dd96185..870b3fd0c101 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
>>   		       index);
>>   		return -EINVAL;
>>   	}
>> +	mutex_lock(&nbd->config_lock);
>>   	if (!refcount_inc_not_zero(&nbd->refs)) {
>> +		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;
>>   	}
>> +	if (!nbd->recv_workq) {
>> +		mutex_unlock(&nbd->config_lock);
>> +		mutex_unlock(&nbd_index_mutex);
>> +		return -EINVAL;
>> +	}
>> +	mutex_unlock(&nbd->config_lock);
>>   	mutex_unlock(&nbd_index_mutex);
>>   	if (!refcount_inc_not_zero(&nbd->config_refs)) {
>>   		nbd_put(nbd);
>>
> 
> With my other patch then we will not need this right? It handles your
> case by just being integrated with the existing checks in:
> 
> nbd_disconnect_and_put->nbd_clear_sock->sock_shutdown
> 
> ...
> 
> static void sock_shutdown(struct nbd_device *nbd)
> {
> 
> ....
> 
>          if (config->num_connections == 0)
>                  return;
> 
> 
> num_connections is zero for your case since we never did a
> nbd_genl_disconnect so we would return here.
> 
> 
> .
> 
Hi Mike

Your point is not right totally.

Yes, config->num_connections is 0 and will return in sock_shutdown. Then 
it will back to nbd_disconnect_and_put and do flush_workqueue 
(nbd->recv_workq).

nbd_disconnect_and_put
	->nbd_clear_sock
		->sock_shutdown
	->flush_workqueue

Thanks,
Sun Ke


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
  2020-02-11  4:12   ` sunke (E)
@ 2020-02-11 16:39     ` Mike Christie
  2020-02-12  2:00       ` sunke (E)
  0 siblings, 1 reply; 6+ messages in thread
From: Mike Christie @ 2020-02-11 16:39 UTC (permalink / raw)
  To: sunke (E), josef, axboe; +Cc: linux-block, nbd, linux-kernel

On 02/10/2020 10:12 PM, sunke (E) wrote:
> 
> 
> 在 2020/2/11 1:05, Mike Christie 写道:
>> On 02/10/2020 01:32 AM, Sun Ke wrote:
>>> Open /dev/nbdX first, the config_refs will be 1 and
>>> the pointers in nbd_device are still null. Disconnect
>>> /dev/nbdX, then reference a null recv_workq. The
>>> protection by config_refs in nbd_genl_disconnect is useless.
>>>
>>> To fix it, just add a check for a non null task_recv in
>>> nbd_genl_disconnect.
>>>
>>> Signed-off-by: Sun Ke <sunke32@huawei.com>
>>> ---
>>> v1 -> v2:
>>> Add an omitted mutex_unlock.
>>>
>>> v2 -> v3:
>>> Add nbd->config_lock, suggested by Josef.
>>> ---
>>>   drivers/block/nbd.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>> index b4607dd96185..870b3fd0c101 100644
>>> --- a/drivers/block/nbd.c
>>> +++ b/drivers/block/nbd.c
>>> @@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff
>>> *skb, struct genl_info *info)
>>>                  index);
>>>           return -EINVAL;
>>>       }
>>> +    mutex_lock(&nbd->config_lock);
>>>       if (!refcount_inc_not_zero(&nbd->refs)) {
>>> +        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;
>>>       }
>>> +    if (!nbd->recv_workq) {
>>> +        mutex_unlock(&nbd->config_lock);
>>> +        mutex_unlock(&nbd_index_mutex);
>>> +        return -EINVAL;
>>> +    }
>>> +    mutex_unlock(&nbd->config_lock);
>>>       mutex_unlock(&nbd_index_mutex);
>>>       if (!refcount_inc_not_zero(&nbd->config_refs)) {
>>>           nbd_put(nbd);
>>>
>>
>> With my other patch then we will not need this right? It handles your
>> case by just being integrated with the existing checks in:
>>
>> nbd_disconnect_and_put->nbd_clear_sock->sock_shutdown
>>
>> ...
>>
>> static void sock_shutdown(struct nbd_device *nbd)
>> {
>>
>> ....
>>
>>          if (config->num_connections == 0)
>>                  return;
>>
>>
>> num_connections is zero for your case since we never did a
>> nbd_genl_disconnect so we would return here.
>>
>>
>> .
>>
> Hi Mike
> 
> Your point is not right totally.
> 
> Yes, config->num_connections is 0 and will return in sock_shutdown. Then
> it will back to nbd_disconnect_and_put and do flush_workqueue
> (nbd->recv_workq).
> 
> nbd_disconnect_and_put
>     ->nbd_clear_sock
>         ->sock_shutdown
>     ->flush_workqueue
> 

My patch removed that extra flush_workqueue in nbd_disconnect_and_put.

The idea of the patch was to move the flush calls to when we do
sock_shutdown in the config (connect, disconnect, clear sock) code
paths, because that is the time we know we will need to kill the recv
workers and wait for them to complete so we know they are not still
running when userspace does a new config operation.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
  2020-02-11 16:39     ` Mike Christie
@ 2020-02-12  2:00       ` sunke (E)
  2020-06-01 11:41         ` Sun Ke
  0 siblings, 1 reply; 6+ messages in thread
From: sunke (E) @ 2020-02-12  2:00 UTC (permalink / raw)
  To: Mike Christie, josef, axboe; +Cc: linux-block, nbd, linux-kernel



在 2020/2/12 0:39, Mike Christie 写道:
> On 02/10/2020 10:12 PM, sunke (E) wrote:
>>
>>
>> 在 2020/2/11 1:05, Mike Christie 写道:
>>> On 02/10/2020 01:32 AM, Sun Ke wrote:
>>>> Open /dev/nbdX first, the config_refs will be 1 and
>>>> the pointers in nbd_device are still null. Disconnect
>>>> /dev/nbdX, then reference a null recv_workq. The
>>>> protection by config_refs in nbd_genl_disconnect is useless.
>>>>
>>>> To fix it, just add a check for a non null task_recv in
>>>> nbd_genl_disconnect.
>>>>
>>>> Signed-off-by: Sun Ke <sunke32@huawei.com>
>>>> ---
>>>> v1 -> v2:
>>>> Add an omitted mutex_unlock.
>>>>
>>>> v2 -> v3:
>>>> Add nbd->config_lock, suggested by Josef.
>>>> ---
>>>>    drivers/block/nbd.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>>> index b4607dd96185..870b3fd0c101 100644
>>>> --- a/drivers/block/nbd.c
>>>> +++ b/drivers/block/nbd.c
>>>> @@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff
>>>> *skb, struct genl_info *info)
>>>>                   index);
>>>>            return -EINVAL;
>>>>        }
>>>> +    mutex_lock(&nbd->config_lock);
>>>>        if (!refcount_inc_not_zero(&nbd->refs)) {
>>>> +        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;
>>>>        }
>>>> +    if (!nbd->recv_workq) {
>>>> +        mutex_unlock(&nbd->config_lock);
>>>> +        mutex_unlock(&nbd_index_mutex);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    mutex_unlock(&nbd->config_lock);
>>>>        mutex_unlock(&nbd_index_mutex);
>>>>        if (!refcount_inc_not_zero(&nbd->config_refs)) {
>>>>            nbd_put(nbd);
>>>>
>>>
>>> With my other patch then we will not need this right? It handles your
>>> case by just being integrated with the existing checks in:
>>>
>>> nbd_disconnect_and_put->nbd_clear_sock->sock_shutdown
>>>
>>> ...
>>>
>>> static void sock_shutdown(struct nbd_device *nbd)
>>> {
>>>
>>> ....
>>>
>>>           if (config->num_connections == 0)
>>>                   return;
>>>
>>>
>>> num_connections is zero for your case since we never did a
>>> nbd_genl_disconnect so we would return here.
>>>
>>>
>>> .
>>>
>> Hi Mike
>>
>> Your point is not right totally.
>>
>> Yes, config->num_connections is 0 and will return in sock_shutdown. Then
>> it will back to nbd_disconnect_and_put and do flush_workqueue
>> (nbd->recv_workq).
>>
>> nbd_disconnect_and_put
>>      ->nbd_clear_sock
>>          ->sock_shutdown
>>      ->flush_workqueue
>>
> 
> My patch removed that extra flush_workqueue in nbd_disconnect_and_put.
> 
> The idea of the patch was to move the flush calls to when we do
> sock_shutdown in the config (connect, disconnect, clear sock) code
> paths, because that is the time we know we will need to kill the recv
> workers and wait for them to complete so we know they are not still
> running when userspace does a new config operation.
> 
Yes, I see.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect
  2020-02-12  2:00       ` sunke (E)
@ 2020-06-01 11:41         ` Sun Ke
  0 siblings, 0 replies; 6+ messages in thread
From: Sun Ke @ 2020-06-01 11:41 UTC (permalink / raw)
  To: Mike Christie, josef, axboe; +Cc: linux-block, nbd, linux-kernel



在 2020/2/12 10:00, sunke (E) 写道:
> 
> 
> 在 2020/2/12 0:39, Mike Christie 写道:
>> On 02/10/2020 10:12 PM, sunke (E) wrote:
>>>
>>>
>>> 在 2020/2/11 1:05, Mike Christie 写道:
>>>> On 02/10/2020 01:32 AM, Sun Ke wrote:
>>>>> Open /dev/nbdX first, the config_refs will be 1 and
>>>>> the pointers in nbd_device are still null. Disconnect
>>>>> /dev/nbdX, then reference a null recv_workq. The
>>>>> protection by config_refs in nbd_genl_disconnect is useless.
>>>>>
>>>>> To fix it, just add a check for a non null task_recv in
>>>>> nbd_genl_disconnect.
>>>>>
>>>>> Signed-off-by: Sun Ke <sunke32@huawei.com>
>>>>> ---
>>>>> v1 -> v2:
>>>>> Add an omitted mutex_unlock.
>>>>>
>>>>> v2 -> v3:
>>>>> Add nbd->config_lock, suggested by Josef.
>>>>> ---
>>>>>    drivers/block/nbd.c | 8 ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>>>> index b4607dd96185..870b3fd0c101 100644
>>>>> --- a/drivers/block/nbd.c
>>>>> +++ b/drivers/block/nbd.c
>>>>> @@ -2008,12 +2008,20 @@ static int nbd_genl_disconnect(struct sk_buff
>>>>> *skb, struct genl_info *info)
>>>>>                   index);
>>>>>            return -EINVAL;
>>>>>        }
>>>>> +    mutex_lock(&nbd->config_lock);
>>>>>        if (!refcount_inc_not_zero(&nbd->refs)) {
>>>>> +        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;
>>>>>        }
>>>>> +    if (!nbd->recv_workq) {
>>>>> +        mutex_unlock(&nbd->config_lock);
>>>>> +        mutex_unlock(&nbd_index_mutex);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +    mutex_unlock(&nbd->config_lock);
>>>>>        mutex_unlock(&nbd_index_mutex);
>>>>>        if (!refcount_inc_not_zero(&nbd->config_refs)) {
>>>>>            nbd_put(nbd);
>>>>>
>>>>
>>>> With my other patch then we will not need this right? It handles your
>>>> case by just being integrated with the existing checks in:
>>>>
>>>> nbd_disconnect_and_put->nbd_clear_sock->sock_shutdown
>>>>
>>>> ...
>>>>
>>>> static void sock_shutdown(struct nbd_device *nbd)
>>>> {
>>>>
>>>> ....
>>>>
>>>>           if (config->num_connections == 0)
>>>>                   return;
>>>>
>>>>
>>>> num_connections is zero for your case since we never did a
>>>> nbd_genl_disconnect so we would return here.
>>>>
>>>>
>>>> .
>>>>
>>> Hi Mike
>>>
>>> Your point is not right totally.
>>>
>>> Yes, config->num_connections is 0 and will return in sock_shutdown. Then
>>> it will back to nbd_disconnect_and_put and do flush_workqueue
>>> (nbd->recv_workq).
>>>
>>> nbd_disconnect_and_put
>>>      ->nbd_clear_sock
>>>          ->sock_shutdown
>>>      ->flush_workqueue
>>>
>>
>> My patch removed that extra flush_workqueue in nbd_disconnect_and_put.
>>
>> The idea of the patch was to move the flush calls to when we do
>> sock_shutdown in the config (connect, disconnect, clear sock) code
>> paths, because that is the time we know we will need to kill the recv
>> workers and wait for them to complete so we know they are not still
>> running when userspace does a new config operation.
>>
> Yes, I see.
> 

Hi Mike

Have you sent your patch?


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-01 11:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10  7:32 [v3] nbd: fix potential NULL pointer fault in nbd_genl_disconnect Sun Ke
2020-02-10 17:05 ` Mike Christie
2020-02-11  4:12   ` sunke (E)
2020-02-11 16:39     ` Mike Christie
2020-02-12  2:00       ` sunke (E)
2020-06-01 11:41         ` Sun Ke

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).