The kmalloc is called with | __GFP_NOFAIL so there is no point in
checking the return value - it either returns valid storage or it would
hang/terminate there. But it is not possible to say if the use of
__GFP_NOFAIL is really needed and the check should be removed or
vice-versa (use of __GFP_NOFAIL should be only in exceptional
cases as I understand it and alloc_srq_queue() is called in quite
a few places)
In either way it would need fixing.
Signed-off-by: Nicholas Mc Guire <[email protected]>
Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
---
Found during code review
Patch was compile tested with: x86_64_defconfig + INFINIBAND=y,
INFINIBAND_USER_ACCESS=y, CHELSIO_T4=y, INFINIBAND_CXGB4=y
(with some unrelated sparse warnings)
Q:This also has an interesting dependency with no effect:
Depends on:... (INFINIBAND_USER_ACCESS [=n] || !INFINIBAND_USER_ACCESS [=n])
I assume htat INFINIBAND_USER_ACCESS=y should be required here ?
Patch is against 5.0-rc2 (localversion-next is next-20190118)
drivers/infiniband/hw/cxgb4/qp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index 917ce5c..c2a12ba 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
wr_len = sizeof(*res_wr) + sizeof(*res);
skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
- if (!skb)
- goto err_free_queue;
set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
--
2.1.4
On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
> Q:This also has an interesting dependency with no effect:
> Depends on:... (INFINIBAND_USER_ACCESS [=n] || !INFINIBAND_USER_ACCESS [=n])
> I assume htat INFINIBAND_USER_ACCESS=y should be required here ?
This has the effect to ensure that if USER_ACCESS is a module then so
is cxgb4, otherwise USER_ACCESS can be enabled or disabled
Jason
On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
> The kmalloc is called with | __GFP_NOFAIL so there is no point in
> checking the return value - it either returns valid storage or it would
> hang/terminate there. But it is not possible to say if the use of
> __GFP_NOFAIL is really needed and the check should be removed or
> vice-versa (use of __GFP_NOFAIL should be only in exceptional
> cases as I understand it and alloc_srq_queue() is called in quite
> a few places)
> In either way it would need fixing.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
> ---
Steve? It seems weird to have NOFAIL and then have an error unwind
path, what is the deal here?
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
> index 917ce5c..c2a12ba 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
> wr_len = sizeof(*res_wr) + sizeof(*res);
>
> skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> - if (!skb)
> - goto err_free_queue;
> set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>
> res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
> --
> 2.1.4
>
On 1/23/2019 12:30 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with | __GFP_NOFAIL so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire <[email protected]>
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
>
> Steve? It seems weird to have NOFAIL and then have an error unwind
> path, what is the deal here?
>
>> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
>> index 917ce5c..c2a12ba 100644
>> --- a/drivers/infiniband/hw/cxgb4/qp.c
>> +++ b/drivers/infiniband/hw/cxgb4/qp.c
>> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
>> wr_len = sizeof(*res_wr) + sizeof(*res);
>>
>> skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
>> - if (!skb)
>> - goto err_free_queue;
>> set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>>
>> res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
>> --
>> 2.1.4
>>
The other queue allocations in qp.c don't use __GFP_NOFAIL. So either
leave it and remove the error check as per this patch, or remove the
NOFAIL and leave the check.
I suggest you remove the __GFP_NOFAIL.
Steve.
On 1/23/2019 12:30 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with | __GFP_NOFAIL so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire <[email protected]>
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
>
> Steve? It seems weird to have NOFAIL and then have an error unwind
> path, what is the deal here?
>
>> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
>> index 917ce5c..c2a12ba 100644
>> --- a/drivers/infiniband/hw/cxgb4/qp.c
>> +++ b/drivers/infiniband/hw/cxgb4/qp.c
>> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
>> wr_len = sizeof(*res_wr) + sizeof(*res);
>>
>> skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
>> - if (!skb)
>> - goto err_free_queue;
>> set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>>
>> res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
>> --
>> 2.1.4
>>
The other queue allocations in qp.c don't use __GFP_NOFAIL. So either
leave it and remove the error check as per this patch, or remove the
NOFAIL and leave the check.
I suggest you remove the __GFP_NOFAIL, since I have a recollection that
using it was frowned upon. In this case, if there is no memory
available it is reasonable to return that error to the user creating the
srq...
Steve.
On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
> The kmalloc is called with | __GFP_NOFAIL so there is no point in
> checking the return value - it either returns valid storage or it would
> hang/terminate there. But it is not possible to say if the use of
> __GFP_NOFAIL is really needed and the check should be removed or
> vice-versa (use of __GFP_NOFAIL should be only in exceptional
> cases as I understand it and alloc_srq_queue() is called in quite
> a few places)
> In either way it would need fixing.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
> ---
As per steve's remarkes I revised this to the below and applied it to
for-next
From 4b2d4262ee2ea58df867de1928bf208795344432 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <[email protected]>
Date: Sun, 20 Jan 2019 02:27:13 +0100
Subject: [PATCH] RDMA/iw_cxgb4: Drop __GFP_NOFAIL
There is no reason for this __GFP_NOFAIL, none of the other routines in
this file use it, and there is an error unwind here. NOFAIL should be
reserved for special cases, not used by network drivers.
Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
Reported-by: Nicholas Mc Guire <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/infiniband/hw/cxgb4/qp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index 03f4c66c265946..c00a4114412694 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -2597,7 +2597,7 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
/* build fw_ri_res_wr */
wr_len = sizeof(*res_wr) + sizeof(*res);
- skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
+ skb = alloc_skb(wr_len, GFP_KERNEL);
if (!skb)
goto err_free_queue;
set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
--
2.20.1
On 1/23/2019 3:44 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with | __GFP_NOFAIL so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire <[email protected]>
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
>
> As per steve's remarkes I revised this to the below and applied it to
> for-next
>
>>From 4b2d4262ee2ea58df867de1928bf208795344432 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <[email protected]>
> Date: Sun, 20 Jan 2019 02:27:13 +0100
> Subject: [PATCH] RDMA/iw_cxgb4: Drop __GFP_NOFAIL
>
> There is no reason for this __GFP_NOFAIL, none of the other routines in
> this file use it, and there is an error unwind here. NOFAIL should be
> reserved for special cases, not used by network drivers.
>
> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
> Reported-by: Nicholas Mc Guire <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> ---
> drivers/infiniband/hw/cxgb4/qp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
> index 03f4c66c265946..c00a4114412694 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2597,7 +2597,7 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
> /* build fw_ri_res_wr */
> wr_len = sizeof(*res_wr) + sizeof(*res);
>
> - skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> + skb = alloc_skb(wr_len, GFP_KERNEL);
> if (!skb)
> goto err_free_queue;
> set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>
Thanks Jason!
On Wed, Jan 23, 2019 at 12:45:11PM -0600, Steve Wise wrote:
>
>
> On 1/23/2019 12:30 PM, Jason Gunthorpe wrote:
> > On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
> >> The kmalloc is called with | __GFP_NOFAIL so there is no point in
> >> checking the return value - it either returns valid storage or it would
> >> hang/terminate there. But it is not possible to say if the use of
> >> __GFP_NOFAIL is really needed and the check should be removed or
> >> vice-versa (use of __GFP_NOFAIL should be only in exceptional
> >> cases as I understand it and alloc_srq_queue() is called in quite
> >> a few places)
> >> In either way it would need fixing.
> >>
> >> Signed-off-by: Nicholas Mc Guire <[email protected]>
> >> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
> >> ---
> >
> > Steve? It seems weird to have NOFAIL and then have an error unwind
> > path, what is the deal here?
> >
> >> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
> >> index 917ce5c..c2a12ba 100644
> >> --- a/drivers/infiniband/hw/cxgb4/qp.c
> >> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> >> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct c4iw_dev_ucontext *uctx,
> >> wr_len = sizeof(*res_wr) + sizeof(*res);
> >>
> >> skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> >> - if (!skb)
> >> - goto err_free_queue;
> >> set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
> >>
> >> res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
> >> --
> >> 2.1.4
> >>
>
> The other queue allocations in qp.c don't use __GFP_NOFAIL. So either
> leave it and remove the error check as per this patch, or remove the
> NOFAIL and leave the check.
>
> I suggest you remove the __GFP_NOFAIL, since I have a recollection that
> using it was frowned upon. In this case, if there is no memory
> available it is reasonable to return that error to the user creating the
> srq...
>
thanks for taking care of this - I simply did not have enough
context to decide if there would be some special reason
for this allocation to need __GFP_NOFAIL - keeping its use
to a minimum though is the best solution.
thx!
hofrat