* Re: [PATCH v2] scsi/cxgbi/libcxgbi: make sure sg is present before calling sg_next()
[not found] <20221114150923.3544796-1-d-tatianin@yandex-team.ru>
@ 2022-11-14 22:23 ` Mike Christie
[not found] ` <84a02b26-392e-994d-bd1f-ecc28231fbfc@yandex-team.ru>
0 siblings, 1 reply; 3+ messages in thread
From: Mike Christie @ 2022-11-14 22:23 UTC (permalink / raw)
To: Daniil Tatianin, James E.J. Bottomley
Cc: Martin K. Petersen, Lee Duncan, Nilesh Javali, Wu Bo, linux-scsi,
linux-kernel, lvc-project, yc-core
On 11/14/22 9:09 AM, Daniil Tatianin wrote:
> sg_next() dereferences the passed sg, therefore we have to verify that
> it's present before calling it.
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE
> static analysis tool.
>
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---
> drivers/scsi/cxgbi/libcxgbi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
> index af281e271f88..2ff9810f42a9 100644
> --- a/drivers/scsi/cxgbi/libcxgbi.c
> +++ b/drivers/scsi/cxgbi/libcxgbi.c
> @@ -1196,8 +1196,7 @@ void cxgbi_ddp_set_one_ppod(struct cxgbi_pagepod *ppod,
>
> if (offset == len) {
> offset = 0;
> - sg = sg_next(sg);
> - if (sg) {
> + if (sg && (sg = sg_next(sg))) {
> addr = sg_dma_address(sg);
> len = sg_dma_len(sg);
> }
Is cxgbit_set_one_ppod the same function but it already has the extra
sg check? Should it be a libcxgb function in libcxgb_ppm.c?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] scsi/cxgbi/libcxgbi: make sure sg is present before calling sg_next()
[not found] ` <84a02b26-392e-994d-bd1f-ecc28231fbfc@yandex-team.ru>
@ 2022-11-15 16:59 ` Mike Christie
[not found] ` <20075199-1e6c-28ad-1440-4dc61985d8f4@yandex-team.ru>
0 siblings, 1 reply; 3+ messages in thread
From: Mike Christie @ 2022-11-15 16:59 UTC (permalink / raw)
To: Daniil Tatianin, James E.J. Bottomley
Cc: Martin K. Petersen, Lee Duncan, Nilesh Javali, Wu Bo, linux-scsi,
linux-kernel, lvc-project, yc-core, Varun Prakash
Cc'ing the cxgbi/t maintainer, Varun.
On 11/15/22 2:17 AM, Daniil Tatianin wrote:
> On 11/15/22 1:23 AM, Mike Christie wrote:
>> On 11/14/22 9:09 AM, Daniil Tatianin wrote:
>>> sg_next() dereferences the passed sg, therefore we have to verify that
>>> it's present before calling it.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE
>>> static analysis tool.
>>>
>>> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>>> ---
>>> drivers/scsi/cxgbi/libcxgbi.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
>>> index af281e271f88..2ff9810f42a9 100644
>>> --- a/drivers/scsi/cxgbi/libcxgbi.c
>>> +++ b/drivers/scsi/cxgbi/libcxgbi.c
>>> @@ -1196,8 +1196,7 @@ void cxgbi_ddp_set_one_ppod(struct cxgbi_pagepod *ppod,
>>> if (offset == len) {
>>> offset = 0;
>>> - sg = sg_next(sg);
>>> - if (sg) {
>>> + if (sg && (sg = sg_next(sg))) {
>>> addr = sg_dma_address(sg);
>>> len = sg_dma_len(sg);
>>> }
>>
>> Is cxgbit_set_one_ppod the same function but it already has the extra
>> sg check?
>
> Good catch! Certainly looks that way, albeit with messier indentation.
>
>> Should it be a libcxgb function in libcxgb_ppm.c?
>
> That makes sense to me. Should I just move both there?
I think you can move one function with a fix to libcxgb and kill the second one.
Name the new function to cxgb_ddp_set_one_ppod then have cxgbi and cxgbt use it.
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v2] scsi/cxgbi/libcxgbi: make sure sg is present before calling sg_next()
[not found] ` <20075199-1e6c-28ad-1440-4dc61985d8f4@yandex-team.ru>
@ 2022-11-17 13:30 ` Varun Prakash
0 siblings, 0 replies; 3+ messages in thread
From: Varun Prakash @ 2022-11-17 13:30 UTC (permalink / raw)
To: Daniil Tatianin, Mike Christie, James E.J. Bottomley
Cc: Martin K. Petersen, Lee Duncan, Nilesh Javali, Wu Bo, linux-scsi,
linux-kernel, lvc-project, yc-core
>>>>> diff --git a/drivers/scsi/cxgbi/libcxgbi.c
>>>>> b/drivers/scsi/cxgbi/libcxgbi.c index af281e271f88..2ff9810f42a9
>>>>> 100644
>>>>> --- a/drivers/scsi/cxgbi/libcxgbi.c
>>>>> +++ b/drivers/scsi/cxgbi/libcxgbi.c
>>>>> @@ -1196,8 +1196,7 @@ void cxgbi_ddp_set_one_ppod(struct
>>>>> cxgbi_pagepod *ppod,
>>>>> if (offset == len) {
>>>>> offset = 0;
>>>>> - sg = sg_next(sg);
>>>>> - if (sg) {
>>>>> + if (sg && (sg = sg_next(sg))) {
>>>>> addr = sg_dma_address(sg);
>>>>> len = sg_dma_len(sg);
>>>>> }
>>>>
>>>> Is cxgbit_set_one_ppod the same function but it already has the
>>>> extra sg check?
>>>
>>> Good catch! Certainly looks that way, albeit with messier indentation.
>>>
>>>> Should it be a libcxgb function in libcxgb_ppm.c?
>>>
>>> That makes sense to me. Should I just move both there?
>>
>> I think you can move one function with a fix to libcxgb and kill the second one.
>> Name the new function to cxgb_ddp_set_one_ppod then have cxgbi and cxgbt use it.
>
>Yeah, thah's pretty much what I meant. Thank you!
Yes, you can move cxgbit_set_one_ppod() from cxgbit_ddp.c to libcxgb_ppm.c and
rename it to cxgbi_ppm_set_one_ppod() and remove cxgbi_ddp_set_one_ppod()
from libcxgbi.c.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-17 13:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20221114150923.3544796-1-d-tatianin@yandex-team.ru>
2022-11-14 22:23 ` [PATCH v2] scsi/cxgbi/libcxgbi: make sure sg is present before calling sg_next() Mike Christie
[not found] ` <84a02b26-392e-994d-bd1f-ecc28231fbfc@yandex-team.ru>
2022-11-15 16:59 ` Mike Christie
[not found] ` <20075199-1e6c-28ad-1440-4dc61985d8f4@yandex-team.ru>
2022-11-17 13:30 ` Varun Prakash
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.