* [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head @ 2022-03-31 21:55 Jakob Koschel 2022-03-31 21:55 ` [PATCH 2/2] cifs: remove check of list iterator against head past the loop body Jakob Koschel 2022-04-01 22:44 ` [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Steve French 0 siblings, 2 replies; 4+ messages in thread From: Jakob Koschel @ 2022-03-31 21:55 UTC (permalink / raw) To: Steve French Cc: linux-cifs, samba-technical, linux-kernel, Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J., 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] cifs: remove check of list iterator against head past the loop body 2022-03-31 21:55 [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Jakob Koschel @ 2022-03-31 21:55 ` Jakob Koschel 2022-04-01 22:44 ` [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Steve French 1 sibling, 0 replies; 4+ messages in thread From: Jakob Koschel @ 2022-03-31 21:55 UTC (permalink / raw) To: Steve French Cc: linux-cifs, samba-technical, linux-kernel, Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J., 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head 2022-03-31 21:55 [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Jakob Koschel 2022-03-31 21:55 ` [PATCH 2/2] cifs: remove check of list iterator against head past the loop body Jakob Koschel @ 2022-04-01 22:44 ` Steve French 2022-04-01 22:57 ` Jakob Koschel 1 sibling, 1 reply; 4+ messages in thread From: Steve French @ 2022-04-01 22:44 UTC (permalink / raw) To: Jakob Koschel Cc: Steve French, CIFS, samba-technical, LKML, Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J. 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head 2022-04-01 22:44 ` [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Steve French @ 2022-04-01 22:57 ` Jakob Koschel 0 siblings, 0 replies; 4+ messages in thread From: Jakob Koschel @ 2022-04-01 22:57 UTC (permalink / raw) To: Steve French Cc: Steve French, CIFS, samba-technical, LKML, Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos, H.J. 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-01 22:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-31 21:55 [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Jakob Koschel 2022-03-31 21:55 ` [PATCH 2/2] cifs: remove check of list iterator against head past the loop body Jakob Koschel 2022-04-01 22:44 ` [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head Steve French 2022-04-01 22:57 ` Jakob Koschel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox