* [PATCH net] net/smc: cancel event worker during device removal
@ 2020-03-06 13:45 Karsten Graul
2020-03-08 15:01 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Karsten Graul @ 2020-03-06 13:45 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390, heiko.carstens, raspl, ubraun
During IB device removal, cancel the event worker before the device
structure is freed. In the worker, check if the device is being
terminated and do not proceed with the event work in that case.
Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
---
net/smc/smc_ib.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index d6ba186f67e2..5e4e64a9aa4b 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
work, struct smc_ib_device, port_event_work);
u8 port_idx;
+ if (list_empty(&smcibdev->list))
+ return;
+
for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
smc_ib_remember_port_attr(smcibdev, port_idx + 1);
clear_bit(port_idx, &smcibdev->port_event_mask);
@@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
smc_smcr_terminate_all(smcibdev);
smc_ib_cleanup_per_ibdev(smcibdev);
ib_unregister_event_handler(&smcibdev->event_handler);
+ cancel_work_sync(&smcibdev->port_event_work);
kfree(smcibdev);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: cancel event worker during device removal
2020-03-06 13:45 [PATCH net] net/smc: cancel event worker during device removal Karsten Graul
@ 2020-03-08 15:01 ` Leon Romanovsky
2020-03-08 19:59 ` Karsten Graul
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2020-03-08 15:01 UTC (permalink / raw)
To: Karsten Graul; +Cc: davem, netdev, linux-s390, heiko.carstens, raspl, ubraun
On Fri, Mar 06, 2020 at 02:45:18PM +0100, Karsten Graul wrote:
> During IB device removal, cancel the event worker before the device
> structure is freed. In the worker, check if the device is being
> terminated and do not proceed with the event work in that case.
>
> Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
> Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
> ---
> net/smc/smc_ib.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index d6ba186f67e2..5e4e64a9aa4b 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
> @@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
> work, struct smc_ib_device, port_event_work);
> u8 port_idx;
>
> + if (list_empty(&smcibdev->list))
> + return;
> +
How can it be true if you are not holding "smc_ib_devices.lock" during
execution of smc_ib_port_event_work()?
> for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
> smc_ib_remember_port_attr(smcibdev, port_idx + 1);
> clear_bit(port_idx, &smcibdev->port_event_mask);
> @@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
> smc_smcr_terminate_all(smcibdev);
> smc_ib_cleanup_per_ibdev(smcibdev);
> ib_unregister_event_handler(&smcibdev->event_handler);
> + cancel_work_sync(&smcibdev->port_event_work);
> kfree(smcibdev);
> }
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: cancel event worker during device removal
2020-03-08 15:01 ` Leon Romanovsky
@ 2020-03-08 19:59 ` Karsten Graul
2020-03-09 8:04 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Karsten Graul @ 2020-03-08 19:59 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: davem, netdev, linux-s390, heiko.carstens, raspl, ubraun
On 08/03/2020 16:01, Leon Romanovsky wrote:
> On Fri, Mar 06, 2020 at 02:45:18PM +0100, Karsten Graul wrote:
>> During IB device removal, cancel the event worker before the device
>> structure is freed. In the worker, check if the device is being
>> terminated and do not proceed with the event work in that case.
>>
>> Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
>> Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
>> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
>> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
>> ---
>> net/smc/smc_ib.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
>> index d6ba186f67e2..5e4e64a9aa4b 100644
>> --- a/net/smc/smc_ib.c
>> +++ b/net/smc/smc_ib.c
>> @@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
>> work, struct smc_ib_device, port_event_work);
>> u8 port_idx;
>>
>> + if (list_empty(&smcibdev->list))
>> + return;
>> +
>
> How can it be true if you are not holding "smc_ib_devices.lock" during
> execution of smc_ib_port_event_work()?
>
It is true when smc_ib_remove_dev() runs before the work actually started.
Other than that its only a shortcut to return earlier, when the item is
removed from the list after the check then the processing just takes a
little bit longer...its still save.
>> for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
>> smc_ib_remember_port_attr(smcibdev, port_idx + 1);
>> clear_bit(port_idx, &smcibdev->port_event_mask);
>> @@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
>> smc_smcr_terminate_all(smcibdev);
>> smc_ib_cleanup_per_ibdev(smcibdev);
>> ib_unregister_event_handler(&smcibdev->event_handler);
>> + cancel_work_sync(&smcibdev->port_event_work);
>> kfree(smcibdev);
>> }
>>
>> --
>> 2.17.1
>>
--
Karsten
(I'm a dude)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: cancel event worker during device removal
2020-03-08 19:59 ` Karsten Graul
@ 2020-03-09 8:04 ` Leon Romanovsky
2020-03-09 9:40 ` Karsten Graul
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2020-03-09 8:04 UTC (permalink / raw)
To: Karsten Graul; +Cc: davem, netdev, linux-s390, heiko.carstens, raspl, ubraun
On Sun, Mar 08, 2020 at 08:59:33PM +0100, Karsten Graul wrote:
> On 08/03/2020 16:01, Leon Romanovsky wrote:
> > On Fri, Mar 06, 2020 at 02:45:18PM +0100, Karsten Graul wrote:
> >> During IB device removal, cancel the event worker before the device
> >> structure is freed. In the worker, check if the device is being
> >> terminated and do not proceed with the event work in that case.
> >>
> >> Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
> >> Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
> >> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
> >> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
> >> ---
> >> net/smc/smc_ib.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> >> index d6ba186f67e2..5e4e64a9aa4b 100644
> >> --- a/net/smc/smc_ib.c
> >> +++ b/net/smc/smc_ib.c
> >> @@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
> >> work, struct smc_ib_device, port_event_work);
> >> u8 port_idx;
> >>
> >> + if (list_empty(&smcibdev->list))
> >> + return;
> >> +
> >
> > How can it be true if you are not holding "smc_ib_devices.lock" during
> > execution of smc_ib_port_event_work()?
> >
>
> It is true when smc_ib_remove_dev() runs before the work actually started.
> Other than that its only a shortcut to return earlier, when the item is
> removed from the list after the check then the processing just takes a
> little bit longer...its still save.
The check itself maybe safe, but it can't fix syzkaller bug reported above.
As you said, the smc_ib_remove_dev() can be called immediately after
your list_empty() check and we return to original behavior.
The correct design will be to ensure that smc_ib_port_event_work() is
executed only smcibdev->list is not empty.
Thanks
>
> >> for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
> >> smc_ib_remember_port_attr(smcibdev, port_idx + 1);
> >> clear_bit(port_idx, &smcibdev->port_event_mask);
> >> @@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
> >> smc_smcr_terminate_all(smcibdev);
> >> smc_ib_cleanup_per_ibdev(smcibdev);
> >> ib_unregister_event_handler(&smcibdev->event_handler);
> >> + cancel_work_sync(&smcibdev->port_event_work);
> >> kfree(smcibdev);
> >> }
> >>
> >> --
> >> 2.17.1
> >>
>
> --
> Karsten
>
> (I'm a dude)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: cancel event worker during device removal
2020-03-09 8:04 ` Leon Romanovsky
@ 2020-03-09 9:40 ` Karsten Graul
2020-03-09 13:19 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Karsten Graul @ 2020-03-09 9:40 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: davem, netdev, linux-s390, heiko.carstens, raspl, ubraun
On 09/03/2020 09:04, Leon Romanovsky wrote:
> On Sun, Mar 08, 2020 at 08:59:33PM +0100, Karsten Graul wrote:
>> On 08/03/2020 16:01, Leon Romanovsky wrote:
>>> On Fri, Mar 06, 2020 at 02:45:18PM +0100, Karsten Graul wrote:
>>>> During IB device removal, cancel the event worker before the device
>>>> structure is freed. In the worker, check if the device is being
>>>> terminated and do not proceed with the event work in that case.
>>>>
>>>> Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
>>>> Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
>>>> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
>>>> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
>>>> ---
>>>> net/smc/smc_ib.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
>>>> index d6ba186f67e2..5e4e64a9aa4b 100644
>>>> --- a/net/smc/smc_ib.c
>>>> +++ b/net/smc/smc_ib.c
>>>> @@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
>>>> work, struct smc_ib_device, port_event_work);
>>>> u8 port_idx;
>>>>
>>>> + if (list_empty(&smcibdev->list))
>>>> + return;
>>>> +
>>>
>>> How can it be true if you are not holding "smc_ib_devices.lock" during
>>> execution of smc_ib_port_event_work()?
>>>
>>
>> It is true when smc_ib_remove_dev() runs before the work actually started.
>> Other than that its only a shortcut to return earlier, when the item is
>> removed from the list after the check then the processing just takes a
>> little bit longer...its still save.
>
> The check itself maybe safe, but it can't fix syzkaller bug reported above.
> As you said, the smc_ib_remove_dev() can be called immediately after
> your list_empty() check and we return to original behavior.
>
> The correct design will be to ensure that smc_ib_port_event_work() is
> executed only smcibdev->list is not empty.
>
> Thanks
>
The fix I had in mind was the
cancel_work_sync(&smcibdev->port_event_work);
to wait for a running port_event_work to finish before smcibdev is freed.
I can remove the list_empty() check if that is too confusing.
>>
>>>> for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
>>>> smc_ib_remember_port_attr(smcibdev, port_idx + 1);
>>>> clear_bit(port_idx, &smcibdev->port_event_mask);
>>>> @@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
>>>> smc_smcr_terminate_all(smcibdev);
>>>> smc_ib_cleanup_per_ibdev(smcibdev);
>>>> ib_unregister_event_handler(&smcibdev->event_handler);
>>>> + cancel_work_sync(&smcibdev->port_event_work);
>>>> kfree(smcibdev);
>>>> }
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>
>> --
>> Karsten
>>
>> (I'm a dude)
>>
--
Karsten
(I'm a dude)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net/smc: cancel event worker during device removal
2020-03-09 9:40 ` Karsten Graul
@ 2020-03-09 13:19 ` Leon Romanovsky
0 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2020-03-09 13:19 UTC (permalink / raw)
To: Karsten Graul; +Cc: davem, netdev, linux-s390, heiko.carstens, raspl, ubraun
On Mon, Mar 09, 2020 at 10:40:16AM +0100, Karsten Graul wrote:
> On 09/03/2020 09:04, Leon Romanovsky wrote:
> > On Sun, Mar 08, 2020 at 08:59:33PM +0100, Karsten Graul wrote:
> >> On 08/03/2020 16:01, Leon Romanovsky wrote:
> >>> On Fri, Mar 06, 2020 at 02:45:18PM +0100, Karsten Graul wrote:
> >>>> During IB device removal, cancel the event worker before the device
> >>>> structure is freed. In the worker, check if the device is being
> >>>> terminated and do not proceed with the event work in that case.
> >>>>
> >>>> Fixes: a4cf0443c414 ("smc: introduce SMC as an IB-client")
> >>>> Reported-by: syzbot+b297c6825752e7a07272@syzkaller.appspotmail.com
> >>>> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
> >>>> Reviewed-by: Ursula Braun <ubraun@linux.ibm.com>
> >>>> ---
> >>>> net/smc/smc_ib.c | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> >>>> index d6ba186f67e2..5e4e64a9aa4b 100644
> >>>> --- a/net/smc/smc_ib.c
> >>>> +++ b/net/smc/smc_ib.c
> >>>> @@ -240,6 +240,9 @@ static void smc_ib_port_event_work(struct work_struct *work)
> >>>> work, struct smc_ib_device, port_event_work);
> >>>> u8 port_idx;
> >>>>
> >>>> + if (list_empty(&smcibdev->list))
> >>>> + return;
> >>>> +
> >>>
> >>> How can it be true if you are not holding "smc_ib_devices.lock" during
> >>> execution of smc_ib_port_event_work()?
> >>>
> >>
> >> It is true when smc_ib_remove_dev() runs before the work actually started.
> >> Other than that its only a shortcut to return earlier, when the item is
> >> removed from the list after the check then the processing just takes a
> >> little bit longer...its still save.
> >
> > The check itself maybe safe, but it can't fix syzkaller bug reported above.
> > As you said, the smc_ib_remove_dev() can be called immediately after
> > your list_empty() check and we return to original behavior.
> >
> > The correct design will be to ensure that smc_ib_port_event_work() is
> > executed only smcibdev->list is not empty.
> >
> > Thanks
> >
>
> The fix I had in mind was the
>
> cancel_work_sync(&smcibdev->port_event_work);
>
> to wait for a running port_event_work to finish before smcibdev is freed.
> I can remove the list_empty() check if that is too confusing.
Yes, please.
Thanks
>
> >>
> >>>> for_each_set_bit(port_idx, &smcibdev->port_event_mask, SMC_MAX_PORTS) {
> >>>> smc_ib_remember_port_attr(smcibdev, port_idx + 1);
> >>>> clear_bit(port_idx, &smcibdev->port_event_mask);
> >>>> @@ -582,6 +585,7 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
> >>>> smc_smcr_terminate_all(smcibdev);
> >>>> smc_ib_cleanup_per_ibdev(smcibdev);
> >>>> ib_unregister_event_handler(&smcibdev->event_handler);
> >>>> + cancel_work_sync(&smcibdev->port_event_work);
> >>>> kfree(smcibdev);
> >>>> }
> >>>>
> >>>> --
> >>>> 2.17.1
> >>>>
> >>
> >> --
> >> Karsten
> >>
> >> (I'm a dude)
> >>
>
> --
> Karsten
>
> (I'm a dude)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-09 13:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06 13:45 [PATCH net] net/smc: cancel event worker during device removal Karsten Graul
2020-03-08 15:01 ` Leon Romanovsky
2020-03-08 19:59 ` Karsten Graul
2020-03-09 8:04 ` Leon Romanovsky
2020-03-09 9:40 ` Karsten Graul
2020-03-09 13:19 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.