2022-04-02 14:29:58

by Jakob Koschel

Subject: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator variable will *always* be a
bogus pointer computed based on the head element.

To avoid type confusion use the actual list head directly instead of
the last iterator value.

Signed-off-by: Jakob Koschel <[email protected]>
---
fs/cifs/smb2pdu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 7e7909b1ae11..4ac86b77a7c9 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
if (!tcon) {
resched = true;
- list_del_init(&ses->rlist);
+ list_del_init(&pserver->smb_ses_list);
cifs_put_smb_ses(ses);
goto done;
}

base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
--
2.25.1

2022-04-04 03:26:43

by Steve French

Subject: Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head

It looks like this no longer applies cleanly. Can you recheck
fs/cifs/smb2pdu.c (function smb2_reconnect_server) and see if it
applies now that it has changed e.g.


/* allocate a dummy tcon struct used for reconnect */
tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
if (!tcon) {
resched = true;
list_for_each_entry_safe(ses, ses2, &tmp_ses_list, rlist) {
list_del_init(&ses->rlist);
cifs_put_smb_ses(ses);
}
goto done;
}

You had this:

--- fs/cifs/smb2pdu.c
+++ fs/cifs/smb2pdu.c
@@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
if (!tcon) {
resched = true;
- list_del_init(&ses->rlist);
+ list_del_init(&pserver->smb_ses_list);
cifs_put_smb_ses(ses);
goto done;
}

On Fri, Apr 1, 2022 at 2:23 AM Jakob Koschel <[email protected]> wrote:
>
> When list_for_each_entry() completes the iteration over the whole list
> without breaking the loop, the iterator variable will *always* be a
> bogus pointer computed based on the head element.
>
> To avoid type confusion use the actual list head directly instead of
> the last iterator value.
>
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> fs/cifs/smb2pdu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 7e7909b1ae11..4ac86b77a7c9 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
> tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> if (!tcon) {
> resched = true;
> - list_del_init(&ses->rlist);
> + list_del_init(&pserver->smb_ses_list);
> cifs_put_smb_ses(ses);
> goto done;
> }
>
> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
> --
> 2.25.1
>


--
Thanks,

Steve

2022-04-05 02:27:50

by Jakob Koschel

Subject: Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head

Hey Steve,

> On 2. Apr 2022, at 00:44, Steve French <[email protected]> wrote:
>
> It looks like this no longer applies cleanly. Can you recheck
> fs/cifs/smb2pdu.c (function smb2_reconnect_server) and see if it
> applies now that it has changed e.g.

looks like this issue was already fixed in the meantime.
Feel free to ignore PATCH 1/2.

>
>
> /* allocate a dummy tcon struct used for reconnect */
> tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> if (!tcon) {
> resched = true;
> list_for_each_entry_safe(ses, ses2, &tmp_ses_list, rlist) {
> list_del_init(&ses->rlist);
> cifs_put_smb_ses(ses);
> }
> goto done;
> }
>
> You had this:
>
> --- fs/cifs/smb2pdu.c
> +++ fs/cifs/smb2pdu.c
> @@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
> tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> if (!tcon) {
> resched = true;
> - list_del_init(&ses->rlist);
> + list_del_init(&pserver->smb_ses_list);
> cifs_put_smb_ses(ses);
> goto done;
> }
>
> On Fri, Apr 1, 2022 at 2:23 AM Jakob Koschel <[email protected]> wrote:
>>
>> When list_for_each_entry() completes the iteration over the whole list
>> without breaking the loop, the iterator variable will *always* be a
>> bogus pointer computed based on the head element.
>>
>> To avoid type confusion use the actual list head directly instead of
>> the last iterator value.
>>
>> Signed-off-by: Jakob Koschel <[email protected]>
>> ---
>> fs/cifs/smb2pdu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 7e7909b1ae11..4ac86b77a7c9 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
>> tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
>> if (!tcon) {
>> resched = true;
>> - list_del_init(&ses->rlist);
>> + list_del_init(&pserver->smb_ses_list);
>> cifs_put_smb_ses(ses);
>> goto done;
>> }
>>
>> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
>> --
>> 2.25.1
>>
>
>
> --
> Thanks,
>
> Steve