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

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

[Jakob Koschel]

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 <jakobkoschel@gmail.com>
---
 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

[PATCH 2/2] cifs: remove check of list iterator against head past the loop body

[Jakob Koschel]

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

While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.

In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 fs/cifs/smb2misc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index b25623e3fe3d..2d862291fab9 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -150,16 +150,18 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
 		struct smb2_transform_hdr *thdr =
 			(struct smb2_transform_hdr *)buf;
 		struct cifs_ses *ses = NULL;
+		struct cifs_ses *iter;
 
 		/* decrypt frame now that it is completely read in */
 		spin_lock(&cifs_tcp_ses_lock);
-		list_for_each_entry(ses, &srvr->smb_ses_list, smb_ses_list) {
-			if (ses->Suid == le64_to_cpu(thdr->SessionId))
+		list_for_each_entry(iter, &srvr->smb_ses_list, smb_ses_list) {
+			if (iter->Suid == le64_to_cpu(thdr->SessionId)) {
+				ses = iter;
 				break;
+			}
 		}
 		spin_unlock(&cifs_tcp_ses_lock);
-		if (list_entry_is_head(ses, &srvr->smb_ses_list,
-				       smb_ses_list)) {
+		if (!ses) {
 			cifs_dbg(VFS, "no decryption - session id not found\n");
 			return 1;
 		}
-- 
2.25.1

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

[Steve French]

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 <jakobkoschel@gmail.com> 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 <jakobkoschel@gmail.com>
> ---
>  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

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

[Jakob Koschel]

Hey Steve,

> On 2. Apr 2022, at 00:44, Steve French <smfrench@gmail.com> 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 <jakobkoschel@gmail.com> 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 <jakobkoschel@gmail.com>
>> ---
>> 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