Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220331215541.883746-1-jakobkoschel@gmail.com>
In-Reply-To: <20220331215541.883746-1-jakobkoschel@gmail.com>
From:   Steve French <smfrench@gmail.com>
Date:   Fri, 1 Apr 2022 17:44:36 -0500
Message-ID: <CAH2r5mv-GCwR74QEv4WncTN6KMHceCT+YcLX7SnmuZwO67DuFQ@mail.gmail.com>
Subject: Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator
 variable with head
To:     Jakob Koschel <jakobkoschel@gmail.com>
Cc:     Steve French <sfrench@samba.org>,
        CIFS <linux-cifs@vger.kernel.org>,
        samba-technical <samba-technical@lists.samba.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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