[PATCH] smb: client: fix oops due to uninitialised var in smb2_unlink()

[PATCH] smb: client: fix oops due to uninitialised var in smb2_unlink()

[Paulo Alcantara]

If SMB2_open_init() or SMB2_close_init() fails (e.g. reconnect), the
iovs set @rqst will be left uninitialised, hence calling
SMB2_open_free(), SMB2_close_free() or smb2_set_related() on them will
oops.

Fix this by initialising @close_iov and @open_iov before setting them
in @rqst.

Reported-by: Thiago Becker <tbecker@redhat.com>
Fixes: 1cf9f2a6a544 ("smb: client: handle unlink(2) of files open by different clients")
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
Cc: David Howells <dhowells@redhat.com>
Cc: linux-cifs@vger.kernel.org
Cc: stable@vger.kernel.org
---
 fs/smb/client/smb2inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 1c4663ed7e69..5280c5c869ad 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -1216,6 +1216,7 @@ smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
 	memset(resp_buftype, 0, sizeof(resp_buftype));
 	memset(rsp_iov, 0, sizeof(rsp_iov));
 
+	memset(open_iov, 0, sizeof(open_iov));
 	rqst[0].rq_iov = open_iov;
 	rqst[0].rq_nvec = ARRAY_SIZE(open_iov);
 
@@ -1240,14 +1241,15 @@ smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
 	creq = rqst[0].rq_iov[0].iov_base;
 	creq->ShareAccess = FILE_SHARE_DELETE_LE;
 
+	memset(&close_iov, 0, sizeof(close_iov));
 	rqst[1].rq_iov = &close_iov;
 	rqst[1].rq_nvec = 1;
 
 	rc = SMB2_close_init(tcon, server, &rqst[1],
 			     COMPOUND_FID, COMPOUND_FID, false);
+	if (rc)
+		goto err_free;
 	smb2_set_related(&rqst[1]);
-	if (rc)
-		goto err_free;
 
 	if (retries) {
 		/* Back-off before retry */
-- 
2.53.0

Re: [PATCH] smb: client: fix oops due to uninitialised var in smb2_unlink()

[Henrique Carvalho]

Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>

We got a lot of those replay uninitialised bugs. Maybe we should prevent
them by having a replay(func, cond) so we can take advantage of a clean
stack. Opinions?

On Thu, Mar 05, 2026 at 09:57:06PM -0300, Paulo Alcantara wrote:
> If SMB2_open_init() or SMB2_close_init() fails (e.g. reconnect), the
> iovs set @rqst will be left uninitialised, hence calling
> SMB2_open_free(), SMB2_close_free() or smb2_set_related() on them will
> oops.
> 
> Fix this by initialising @close_iov and @open_iov before setting them
> in @rqst.
> 
> Reported-by: Thiago Becker <tbecker@redhat.com>
> Fixes: 1cf9f2a6a544 ("smb: client: handle unlink(2) of files open by different clients")
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: linux-cifs@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  fs/smb/client/smb2inode.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 1c4663ed7e69..5280c5c869ad 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -1216,6 +1216,7 @@ smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
>  	memset(resp_buftype, 0, sizeof(resp_buftype));
>  	memset(rsp_iov, 0, sizeof(rsp_iov));
>  
> +	memset(open_iov, 0, sizeof(open_iov));
>  	rqst[0].rq_iov = open_iov;
>  	rqst[0].rq_nvec = ARRAY_SIZE(open_iov);
>  
> @@ -1240,14 +1241,15 @@ smb2_unlink(const unsigned int xid, struct cifs_tcon *tcon, const char *name,
>  	creq = rqst[0].rq_iov[0].iov_base;
>  	creq->ShareAccess = FILE_SHARE_DELETE_LE;
>  
> +	memset(&close_iov, 0, sizeof(close_iov));
>  	rqst[1].rq_iov = &close_iov;
>  	rqst[1].rq_nvec = 1;
>  
>  	rc = SMB2_close_init(tcon, server, &rqst[1],
>  			     COMPOUND_FID, COMPOUND_FID, false);
> +	if (rc)
> +		goto err_free;
>  	smb2_set_related(&rqst[1]);
> -	if (rc)
> -		goto err_free;
>  
>  	if (retries) {
>  		/* Back-off before retry */
> -- 
> 2.53.0
> 
> 

Re: [PATCH] smb: client: fix oops due to uninitialised var in smb2_unlink()

[Paulo Alcantara]

Henrique Carvalho <henrique.carvalho@suse.com> writes:

> Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
>
> We got a lot of those replay uninitialised bugs. Maybe we should prevent
> them by having a replay(func, cond) so we can take advantage of a clean
> stack. Opinions?

Agreed.  No strong opinions.  I'm wondering if that should be
implemented in the transport layer, therefore we could get rid of all
that duplicate code.  Alternatively, having the thing implemented in
netfslib instead.

Re: [PATCH] smb: client: fix oops due to uninitialised var in smb2_unlink()

[Henrique Carvalho]

On Fri, Mar 06, 2026 at 07:51:44PM -0300, Paulo Alcantara wrote:
> Henrique Carvalho <henrique.carvalho@suse.com> writes:
> 
> > Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >
> > We got a lot of those replay uninitialised bugs. Maybe we should prevent
> > them by having a replay(func, cond) so we can take advantage of a clean
> > stack. Opinions?
> 
> Agreed.  No strong opinions.  I'm wondering if that should be
> implemented in the transport layer, therefore we could get rid of all
> that duplicate code.  Alternatively, having the thing implemented in
> netfslib instead.

Since not everything goes through netfslib, it makes sense to be
implemented in cifs with different approaches to netfslib.

I thought that implementing in transport layer is a good idea. However,
we have two problems here. First, netfslib callbacks also go through
this layer, leaving us with the issue David mentioned about performance
and handing in cifs before returning to netfslib. Second, we would need
to change the cifs_pick_channel architecture as well, because the replay
happens in potentially two different channels.

So I thought about doing something like the following, please give me
your thoughts. This would require a lot of changes in existing replay
functinos, but mostly intialization at the top, removal of internal
replay logic and definition of a static *_once function.

[PATCH] smb: client: introduce smb2_should_replay_wait() helper

Introduce smb2_should_replay_wait() to centralize replay decision and
backoff.

This wraps the existing replay logic so call sites can use:

        int smb2_operation(..., tcon) {
                int rc = 0;
                int retries = 0;
                int cur_sleep = 0;
                do {
                        rc = smb2_operation_once(..., retries > 0);
                } while (smb2_should_replay_wait(tcon, rc, &retries, &cur_sleep));
                return rc;
        }

This helps avoid replay-path uninitialized variable errors by encouraging
single-attempt helpers with fresh locals per retry.

Signed-off-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
 fs/smb/client/smb2ops.c   | 24 ++++++++++++++++++++++++
 fs/smb/client/smb2proto.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 592c1609443b..43223389e2b6 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2806,6 +2806,30 @@ bool smb2_should_replay(struct cifs_tcon *tcon,
 	return false;
 }
 
+/*
+ * smb2_should_replay_wait - decide whether to replay an operation and
+ * back off (may sleep)
+ *
+ * @tcon
+ * @rc: return code from the previous attempt
+ * @retries: retry counter modifiable by smb2_should_replay()
+ * @cur_sleep: backoff delay (ms) modifiable by smb2_should_replay()
+ *
+ * Returns true if the operation should be retried
+ */
+bool smb2_should_replay_wait(struct cifs_tcon *tcon,
+		              int rc,
+		              int *retries,
+		              int *cur_sleep)
+{
+	if (!is_replayable_error(rc))
+		return false;
+	if (!smb2_should_replay(tcon, retries, cur_sleep))
+		return false;
+	if (*cur_sleep)
+		msleep(*cur_sleep);
+	return true;
+}
+