[PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Leon Romanovsky]

From: Jack Morgenstein <jackm@nvidia.com>

The cited commit fixed a crash when cma_netevent_callback was called for
a cma_id while work on that id from a previous call had not yet started.
The work item was re-initialized in the second call, which corrupted the
work item currently in the work queue.

However, it left a problem when queue_work fails (because the item is
still pending in the work queue from a previous call). In this case,
cma_id_put (which is called in the work handler) is therefore not
called. This results in a userspace process hang (zombie process).

Fix this by calling cma_id_put() if queue_work fails.

Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")
Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index ab31eefa916b3..274cfbd5aaba7 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -5245,7 +5245,8 @@ static int cma_netevent_callback(struct notifier_block *self,
 			   neigh->ha, ETH_ALEN))
 			continue;
 		cma_id_get(current_id);
-		queue_work(cma_wq, &current_id->id.net_work);
+		if (!queue_work(cma_wq, &current_id->id.net_work))
+			cma_id_put(current_id);
 	}
 out:
 	spin_unlock_irqrestore(&id_table_lock, flags);
-- 
2.49.0

Re: [PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Sharath Srinivasan]

On 2025-05-21 4:36 a.m., Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@nvidia.com>
> 
> The cited commit fixed a crash when cma_netevent_callback was called for
> a cma_id while work on that id from a previous call had not yet started.
> The work item was re-initialized in the second call, which corrupted the
> work item currently in the work queue.
> 
> However, it left a problem when queue_work fails (because the item is
> still pending in the work queue from a previous call). In this case,
> cma_id_put (which is called in the work handler) is therefore not
> called. This results in a userspace process hang (zombie process).
> 
> Fix this by calling cma_id_put() if queue_work fails.
> 
> Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")

IMO the above Fixes: tag should point to the commit that introduced the line:
"queue_work(cma_wq, &current_id->id.net_work);"

i.e. Fixes: 925d046e7e52 ("RDMA/core: Add a netevent notifier to cma")

and not another bug fix (45f5dcdd0497) which did not introduce the problem being described in this patch (a missing cma_id_put() when queue_work() fails).

Otherwise the fix looks good to me:
Reviewed-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>

Thanks,
Sharath

> Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/core/cma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index ab31eefa916b3..274cfbd5aaba7 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -5245,7 +5245,8 @@ static int cma_netevent_callback(struct notifier_block *self,
>  			   neigh->ha, ETH_ALEN))
>  			continue;
>  		cma_id_get(current_id);
> -		queue_work(cma_wq, &current_id->id.net_work);
> +		if (!queue_work(cma_wq, &current_id->id.net_work))
> +			cma_id_put(current_id);
>  	}
>  out:
>  	spin_unlock_irqrestore(&id_table_lock, flags);

Re: [PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Leon Romanovsky]

On Wed, May 21, 2025 at 11:59:22AM -0700, Sharath Srinivasan wrote:
> 
> On 2025-05-21 4:36 a.m., Leon Romanovsky wrote:
> > From: Jack Morgenstein <jackm@nvidia.com>
> > 
> > The cited commit fixed a crash when cma_netevent_callback was called for
> > a cma_id while work on that id from a previous call had not yet started.
> > The work item was re-initialized in the second call, which corrupted the
> > work item currently in the work queue.
> > 
> > However, it left a problem when queue_work fails (because the item is
> > still pending in the work queue from a previous call). In this case,
> > cma_id_put (which is called in the work handler) is therefore not
> > called. This results in a userspace process hang (zombie process).
> > 
> > Fix this by calling cma_id_put() if queue_work fails.
> > 
> > Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")
> 
> IMO the above Fixes: tag should point to the commit that introduced the line:
> "queue_work(cma_wq, &current_id->id.net_work);"
> 
> i.e. Fixes: 925d046e7e52 ("RDMA/core: Add a netevent notifier to cma")
> 
> and not another bug fix (45f5dcdd0497) which did not introduce the problem being described in this patch (a missing cma_id_put() when queue_work() fails).

It is not, according to the queue_work() description and implementation,
that function call can fail only if this work already exist. Before commit 45f5dcdd0497
that cma_netevent_work was always new and hence can't fail. This is why queue_work()
returned value is almost never checked in the kernel.

Thanks

> 
> Otherwise the fix looks good to me:
> Reviewed-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
> 
> Thanks,
> Sharath
> 
> > Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
> > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/infiniband/core/cma.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index ab31eefa916b3..274cfbd5aaba7 100644
> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -5245,7 +5245,8 @@ static int cma_netevent_callback(struct notifier_block *self,
> >  			   neigh->ha, ETH_ALEN))
> >  			continue;
> >  		cma_id_get(current_id);
> > -		queue_work(cma_wq, &current_id->id.net_work);
> > +		if (!queue_work(cma_wq, &current_id->id.net_work))
> > +			cma_id_put(current_id);
> >  	}
> >  out:
> >  	spin_unlock_irqrestore(&id_table_lock, flags);
> 

Re: [PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Sharath Srinivasan]

On 2025-05-22 1:58 a.m., Leon Romanovsky wrote:
> On Wed, May 21, 2025 at 11:59:22AM -0700, Sharath Srinivasan wrote:
>>
>> On 2025-05-21 4:36 a.m., Leon Romanovsky wrote:
>>> From: Jack Morgenstein <jackm@nvidia.com>
>>>
>>> The cited commit fixed a crash when cma_netevent_callback was called for
>>> a cma_id while work on that id from a previous call had not yet started.
>>> The work item was re-initialized in the second call, which corrupted the
>>> work item currently in the work queue.
>>>
>>> However, it left a problem when queue_work fails (because the item is
>>> still pending in the work queue from a previous call). In this case,
>>> cma_id_put (which is called in the work handler) is therefore not
>>> called. This results in a userspace process hang (zombie process).
>>>
>>> Fix this by calling cma_id_put() if queue_work fails.
>>>
>>> Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")
>>
>> IMO the above Fixes: tag should point to the commit that introduced the line:
>> "queue_work(cma_wq, &current_id->id.net_work);"
>>
>> i.e. Fixes: 925d046e7e52 ("RDMA/core: Add a netevent notifier to cma")
>>
>> and not another bug fix (45f5dcdd0497) which did not introduce the problem being described in this patch (a missing cma_id_put() when queue_work() fails).
> 
> It is not, according to the queue_work() description and implementation,
> that function call can fail only if this work already exist. Before commit 45f5dcdd0497
> that cma_netevent_work was always new and hence can't fail. This is why queue_work()
> returned value is almost never checked in the kernel.
> 
> Thanks
> 

Thanks for clarifying. Makes sense to say "Fixes: 45f5dcdd0497".

Regards,
Sharath

>>
>> Otherwise the fix looks good to me:
>> Reviewed-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
>>
>> Thanks,
>> Sharath
>>
>>> Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>> Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
>>> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>>> ---
>>>  drivers/infiniband/core/cma.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>> index ab31eefa916b3..274cfbd5aaba7 100644
>>> --- a/drivers/infiniband/core/cma.c
>>> +++ b/drivers/infiniband/core/cma.c
>>> @@ -5245,7 +5245,8 @@ static int cma_netevent_callback(struct notifier_block *self,
>>>  			   neigh->ha, ETH_ALEN))
>>>  			continue;
>>>  		cma_id_get(current_id);
>>> -		queue_work(cma_wq, &current_id->id.net_work);
>>> +		if (!queue_work(cma_wq, &current_id->id.net_work))
>>> +			cma_id_put(current_id);
>>>  	}
>>>  out:
>>>  	spin_unlock_irqrestore(&id_table_lock, flags);
>>

Re: [PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Kalesh Anakkur Purayil]

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

On Wed, May 21, 2025 at 5:06 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Jack Morgenstein <jackm@nvidia.com>
>
> The cited commit fixed a crash when cma_netevent_callback was called for
> a cma_id while work on that id from a previous call had not yet started.
> The work item was re-initialized in the second call, which corrupted the
> work item currently in the work queue.
>
> However, it left a problem when queue_work fails (because the item is
> still pending in the work queue from a previous call). In this case,
> cma_id_put (which is called in the work handler) is therefore not
> called. This results in a userspace process hang (zombie process).
>
> Fix this by calling cma_id_put() if queue_work fails.
>
> Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")
> Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
LGTM,
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4226 bytes --]

Re: [PATCH rdma-rc] RDMA/cma: Fix hang when cma_netevent_callback fails to queue_work

[Jason Gunthorpe]

On Wed, May 21, 2025 at 02:36:02PM +0300, Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@nvidia.com>
> 
> The cited commit fixed a crash when cma_netevent_callback was called for
> a cma_id while work on that id from a previous call had not yet started.
> The work item was re-initialized in the second call, which corrupted the
> work item currently in the work queue.
> 
> However, it left a problem when queue_work fails (because the item is
> still pending in the work queue from a previous call). In this case,
> cma_id_put (which is called in the work handler) is therefore not
> called. This results in a userspace process hang (zombie process).
> 
> Fix this by calling cma_id_put() if queue_work fails.
> 
> Fixes: 45f5dcdd0497 ("RDMA/cma: Fix workqueue crash in cma_netevent_work_handler")
> Signed-off-by: Jack Morgenstein <jackm@nvidia.com>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Vlad Dumitrescu <vdumitrescu@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> Reviewed-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> ---
>  drivers/infiniband/core/cma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied to for-next, thanks

Jason