[PATCH 1/4] smb: client: Delete unused value

[PATCH 1/4] smb: client: Delete unused value

[Pierre Mariani]

rc does not need to be set to any value in this location as it gets set to other
values is all subsequent logical branches before being used.
Fixes Coverity 1562035 Unused value.

Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
---
 fs/smb/client/connect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index f896f60c924b..449d56802692 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1770,7 +1770,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
 			tcp_ses, (struct sockaddr *)&ctx->dstaddr);
 		if (tcp_ses->smbd_conn) {
 			cifs_dbg(VFS, "RDMA transport established\n");
-			rc = 0;
 			goto smbd_connected;
 		} else {
 			rc = -ENOENT;
-- 
2.39.2

[PATCH 2/4] smb: client: Protect ses->chans update with chan_lock spin lock

[Pierre Mariani]

Protect the update of ses->chans with chan_lock spin lock as per documentation
from cifsglob.h.
Fixes Coverity 1561738.

Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
---
 fs/smb/client/connect.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 449d56802692..0512835f399c 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2055,6 +2055,7 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	/* close any extra channels */
+	spin_lock(&ses->chan_lock);
 	for (i = 1; i < ses->chan_count; i++) {
 		if (ses->chans[i].iface) {
 			kref_put(&ses->chans[i].iface->refcount, release_iface);
@@ -2063,11 +2064,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
 		cifs_put_tcp_session(ses->chans[i].server, 0);
 		ses->chans[i].server = NULL;
 	}
+	spin_unlock(&ses->chan_lock);
 
 	/* we now account for primary channel in iface->refcount */
 	if (ses->chans[0].iface) {
 		kref_put(&ses->chans[0].iface->refcount, release_iface);
+		spin_lock(&ses->chan_lock);
 		ses->chans[0].server = NULL;
+		spin_unlock(&ses->chan_lock);
 	}
 
 	sesInfoFree(ses);
-- 
2.39.2

[PATCH 3/4] smb: client: Protect tcon->status with tc_lock spin lock

[Pierre Mariani]

Protect the update of tcon->status with tc_lock spin lock as per documentation
from cifsglob.h.
Fixes Coverity 1560722 Data race condition.

Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
---
 fs/smb/client/connect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 0512835f399c..a381c4cdb8c4 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2710,7 +2710,9 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 	tcon->nodelete = ctx->nodelete;
 	tcon->local_lease = ctx->local_lease;
 	INIT_LIST_HEAD(&tcon->pending_opens);
+	spin_lock(&tcon->tc_lock);
 	tcon->status = TID_GOOD;
+	spin_unlock(&tcon->tc_lock);
 
 	INIT_DELAYED_WORK(&tcon->query_interfaces,
 			  smb2_query_server_interfaces);
-- 
2.39.2

[PATCH 4/4] smb: client: Fix checkpatch whitespace errors and warnings

[Pierre Mariani]

Fixes no-op checkpatch errors and warnings.

Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
---
 fs/smb/client/connect.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index a381c4cdb8c4..59f95ea5105e 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -482,6 +482,7 @@ static int reconnect_target_unlocked(struct TCP_Server_Info *server, struct dfs_
 static int reconnect_dfs_server(struct TCP_Server_Info *server)
 {
 	struct dfs_cache_tgt_iterator *target_hint = NULL;
+
 	DFS_CACHE_TGT_LIST(tl);
 	int num_targets = 0;
 	int rc = 0;
@@ -750,6 +751,7 @@ cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
 {
 	struct msghdr smb_msg = {};
 	struct kvec iov = {.iov_base = buf, .iov_len = to_read};
+
 	iov_iter_kvec(&smb_msg.msg_iter, ITER_DEST, &iov, 1, to_read);
 
 	return cifs_readv_from_socket(server, &smb_msg);
@@ -1400,11 +1402,13 @@ cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs)
 	case AF_INET: {
 		struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
 		struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
+
 		return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
 	}
 	case AF_INET6: {
 		struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
 		struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs;
+
 		return (ipv6_addr_equal(&saddr6->sin6_addr, &vaddr6->sin6_addr)
 			&& saddr6->sin6_scope_id == vaddr6->sin6_scope_id);
 	}
@@ -2605,8 +2609,8 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 			rc = -EOPNOTSUPP;
 			goto out_fail;
 		} else {
-			cifs_dbg(VFS, "Check vers= mount option. SMB3.11 "
-				"disabled but required for POSIX extensions\n");
+			cifs_dbg(VFS,
+				"Check vers= mount option. SMB3.11 disabled but required for POSIX extensions\n");
 			rc = -EOPNOTSUPP;
 			goto out_fail;
 		}
@@ -2751,7 +2755,6 @@ cifs_put_tlink(struct tcon_link *tlink)
 	if (!IS_ERR(tlink_tcon(tlink)))
 		cifs_put_tcon(tlink_tcon(tlink));
 	kfree(tlink);
-	return;
 }
 
 static int
@@ -2892,6 +2895,7 @@ static inline void
 cifs_reclassify_socket4(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
+
 	BUG_ON(!sock_allow_reclassification(sk));
 	sock_lock_init_class_and_name(sk, "slock-AF_INET-CIFS",
 		&cifs_slock_key[0], "sk_lock-AF_INET-CIFS", &cifs_key[0]);
@@ -2901,6 +2905,7 @@ static inline void
 cifs_reclassify_socket6(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
+
 	BUG_ON(!sock_allow_reclassification(sk));
 	sock_lock_init_class_and_name(sk, "slock-AF_INET6-CIFS",
 		&cifs_slock_key[1], "sk_lock-AF_INET6-CIFS", &cifs_key[1]);
@@ -2935,15 +2940,18 @@ static int
 bind_socket(struct TCP_Server_Info *server)
 {
 	int rc = 0;
+
 	if (server->srcaddr.ss_family != AF_UNSPEC) {
 		/* Bind to the specified local IP address */
 		struct socket *socket = server->ssocket;
+
 		rc = kernel_bind(socket,
 				 (struct sockaddr *) &server->srcaddr,
 				 sizeof(server->srcaddr));
 		if (rc < 0) {
 			struct sockaddr_in *saddr4;
 			struct sockaddr_in6 *saddr6;
+
 			saddr4 = (struct sockaddr_in *)&server->srcaddr;
 			saddr6 = (struct sockaddr_in6 *)&server->srcaddr;
 			if (saddr6->sin6_family == AF_INET6)
@@ -3173,6 +3181,7 @@ void reset_cifs_unix_caps(unsigned int xid, struct cifs_tcon *tcon,
 
 	if (!CIFSSMBQFSUnixInfo(xid, tcon)) {
 		__u64 cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
+
 		cifs_dbg(FYI, "unix caps which server supports %lld\n", cap);
 		/*
 		 * check for reconnect case in which we do not
@@ -3676,7 +3685,7 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
 	smb_buffer_response = smb_buffer;
 
 	header_assemble(smb_buffer, SMB_COM_TREE_CONNECT_ANDX,
-			NULL /*no tid */ , 4 /*wct */ );
+			NULL /*no tid */, 4 /*wct */);
 
 	smb_buffer->Mid = get_next_mid(ses->server);
 	smb_buffer->Uid = ses->Suid;
@@ -3695,12 +3704,12 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
 	if (ses->server->sign)
 		smb_buffer->Flags2 |= SMBFLG2_SECURITY_SIGNATURE;
 
-	if (ses->capabilities & CAP_STATUS32) {
+	if (ses->capabilities & CAP_STATUS32)
 		smb_buffer->Flags2 |= SMBFLG2_ERR_STATUS;
-	}
-	if (ses->capabilities & CAP_DFS) {
+
+	if (ses->capabilities & CAP_DFS)
 		smb_buffer->Flags2 |= SMBFLG2_DFS;
-	}
+
 	if (ses->capabilities & CAP_UNICODE) {
 		smb_buffer->Flags2 |= SMBFLG2_UNICODE;
 		length =
-- 
2.39.2

Re: [PATCH 1/4] smb: client: Delete unused value

[Steve French]

on this one - I lean toward leaving it in (although technically
unused) since may reduce future errors by being clear that this is not
an error case and may be a bit clearer to read to some.  No strong
opinion though on this.

On Sun, Nov 26, 2023 at 10:52 PM Pierre Mariani
<pierre.mariani@gmail.com> wrote:
>
> rc does not need to be set to any value in this location as it gets set to other
> values is all subsequent logical branches before being used.
> Fixes Coverity 1562035 Unused value.
>
> Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> ---
>  fs/smb/client/connect.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index f896f60c924b..449d56802692 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1770,7 +1770,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
>                         tcp_ses, (struct sockaddr *)&ctx->dstaddr);
>                 if (tcp_ses->smbd_conn) {
>                         cifs_dbg(VFS, "RDMA transport established\n");
> -                       rc = 0;
>                         goto smbd_connected;
>                 } else {
>                         rc = -ENOENT;
> --
> 2.39.2
>


-- 
Thanks,

Steve

Re: [PATCH 1/4] smb: client: Delete unused value

[Pierre Mariani]

On Sun, Nov 26, 2023 at 10:54:38PM -0600, Steve French wrote:
> on this one - I lean toward leaving it in (although technically
> unused) since may reduce future errors by being clear that this is not
> an error case and may be a bit clearer to read to some.  No strong
> opinion though on this.
> 

I will undo this change and update the Coverity triage data instead.

> On Sun, Nov 26, 2023 at 10:52 PM Pierre Mariani
> <pierre.mariani@gmail.com> wrote:
> >
> > rc does not need to be set to any value in this location as it gets set to other
> > values is all subsequent logical branches before being used.
> > Fixes Coverity 1562035 Unused value.
> >
> > Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> > ---
> >  fs/smb/client/connect.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index f896f60c924b..449d56802692 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -1770,7 +1770,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
> >                         tcp_ses, (struct sockaddr *)&ctx->dstaddr);
> >                 if (tcp_ses->smbd_conn) {
> >                         cifs_dbg(VFS, "RDMA transport established\n");
> > -                       rc = 0;
> >                         goto smbd_connected;
> >                 } else {
> >                         rc = -ENOENT;
> > --
> > 2.39.2
> >
> 
> 
> -- 
> Thanks,
> 
> Steve

Re: [PATCH 1/4] smb: client: Delete unused value

[Pierre Mariani]

On Sun, Nov 26, 2023 at 09:15:07PM -0800, Pierre Mariani wrote:
> On Sun, Nov 26, 2023 at 10:54:38PM -0600, Steve French wrote:
> > on this one - I lean toward leaving it in (although technically
> > unused) since may reduce future errors by being clear that this is not
> > an error case and may be a bit clearer to read to some.  No strong
> > opinion though on this.
> > 
> 
> I will undo this change and update the Coverity triage data instead.

I will not be able to make updates to Coverity until I get contributor access,
which will happen if and when one of my patches gets merged. I will update
Coverity whenever it becomes possible.

I have undone the change and submitted a v2 patch set.

> 
> > On Sun, Nov 26, 2023 at 10:52 PM Pierre Mariani
> > <pierre.mariani@gmail.com> wrote:
> > >
> > > rc does not need to be set to any value in this location as it gets set to other
> > > values is all subsequent logical branches before being used.
> > > Fixes Coverity 1562035 Unused value.
> > >
> > > Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> > > ---
> > >  fs/smb/client/connect.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > > index f896f60c924b..449d56802692 100644
> > > --- a/fs/smb/client/connect.c
> > > +++ b/fs/smb/client/connect.c
> > > @@ -1770,7 +1770,6 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
> > >                         tcp_ses, (struct sockaddr *)&ctx->dstaddr);
> > >                 if (tcp_ses->smbd_conn) {
> > >                         cifs_dbg(VFS, "RDMA transport established\n");
> > > -                       rc = 0;
> > >                         goto smbd_connected;
> > >                 } else {
> > >                         rc = -ENOENT;
> > > --
> > > 2.39.2
> > >
> > 
> > 
> > -- 
> > Thanks,
> > 
> > Steve

Re: [PATCH 2/4] smb: client: Protect ses->chans update with chan_lock spin lock

[Shyam Prasad N]

On Mon, Nov 27, 2023 at 10:22 AM Pierre Mariani
<pierre.mariani@gmail.com> wrote:
>
> Protect the update of ses->chans with chan_lock spin lock as per documentation
> from cifsglob.h.
> Fixes Coverity 1561738.
>
> Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> ---
>  fs/smb/client/connect.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 449d56802692..0512835f399c 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -2055,6 +2055,7 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
>         spin_unlock(&cifs_tcp_ses_lock);
>
>         /* close any extra channels */
> +       spin_lock(&ses->chan_lock);
>         for (i = 1; i < ses->chan_count; i++) {
>                 if (ses->chans[i].iface) {
>                         kref_put(&ses->chans[i].iface->refcount, release_iface);
> @@ -2063,11 +2064,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
>                 cifs_put_tcp_session(ses->chans[i].server, 0);
>                 ses->chans[i].server = NULL;
>         }
> +       spin_unlock(&ses->chan_lock);
>
>         /* we now account for primary channel in iface->refcount */
>         if (ses->chans[0].iface) {
>                 kref_put(&ses->chans[0].iface->refcount, release_iface);
> +               spin_lock(&ses->chan_lock);
>                 ses->chans[0].server = NULL;
> +               spin_unlock(&ses->chan_lock);
>         }
>
>         sesInfoFree(ses);
> --
> 2.39.2
>
>

Hi Pierre,

Thanks for proposing this change.

While it is true in general that chan_lock needs to be locked when
dealing with session channel details, this particular instance above
is during __cifs_put_smb_ses.
And this code is reached when ses_count has already reached 0. i.e.
this process is the last user of the session.
So taking chan_lock can be avoided. We did have this under a lock
before, but it resulted in deadlocks due to calls to
cifs_put_tcp_session, which locks bigger locks.
So the quick and dirty fix at that point was to not take chan_lock
here, knowing that we'll be the last user.

Perhaps a better fix exists?
Or we should probably document this as a comment for now.

This version of the patch will result in the deadlocks again.

-- 
Regards,
Shyam

Re: [PATCH 2/4] smb: client: Protect ses->chans update with chan_lock spin lock

[Pierre Mariani]

On Wed, Nov 29, 2023 at 02:30:37PM +0530, Shyam Prasad N wrote:
> On Mon, Nov 27, 2023 at 10:22 AM Pierre Mariani
> <pierre.mariani@gmail.com> wrote:
> >
> > Protect the update of ses->chans with chan_lock spin lock as per documentation
> > from cifsglob.h.
> > Fixes Coverity 1561738.
> >
> > Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> > ---
> >  fs/smb/client/connect.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 449d56802692..0512835f399c 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -2055,6 +2055,7 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >         spin_unlock(&cifs_tcp_ses_lock);
> >
> >         /* close any extra channels */
> > +       spin_lock(&ses->chan_lock);
> >         for (i = 1; i < ses->chan_count; i++) {
> >                 if (ses->chans[i].iface) {
> >                         kref_put(&ses->chans[i].iface->refcount, release_iface);
> > @@ -2063,11 +2064,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >                 cifs_put_tcp_session(ses->chans[i].server, 0);
> >                 ses->chans[i].server = NULL;
> >         }
> > +       spin_unlock(&ses->chan_lock);
> >
> >         /* we now account for primary channel in iface->refcount */
> >         if (ses->chans[0].iface) {
> >                 kref_put(&ses->chans[0].iface->refcount, release_iface);
> > +               spin_lock(&ses->chan_lock);
> >                 ses->chans[0].server = NULL;
> > +               spin_unlock(&ses->chan_lock);
> >         }
> >
> >         sesInfoFree(ses);
> > --
> > 2.39.2
> >
> >
> 
> Hi Pierre,
> 
> Thanks for proposing this change.
> 
> While it is true in general that chan_lock needs to be locked when
> dealing with session channel details, this particular instance above
> is during __cifs_put_smb_ses.
> And this code is reached when ses_count has already reached 0. i.e.
> this process is the last user of the session.
> So taking chan_lock can be avoided. We did have this under a lock
> before, but it resulted in deadlocks due to calls to
> cifs_put_tcp_session, which locks bigger locks.
> So the quick and dirty fix at that point was to not take chan_lock
> here, knowing that we'll be the last user.
> 
> Perhaps a better fix exists?
> Or we should probably document this as a comment for now.
> 
> This version of the patch will result in the deadlocks again.

Thank you for educating me on this, Shyam. I will re-read the code from that
point of view and see if I can think of any improvement.

> 
> -- 
> Regards,
> Shyam

Re: [PATCH 4/4] smb: client: Fix checkpatch whitespace errors and warnings

[Steve French]

patch looks fine other than a trivial checkpatch warning (I corrected).

Although trivial style changes - they look non-controversial and small

$ ./scripts/checkpatch.pl
0001-smb-client-Fix-checkpatch-whitespace-errors-and-warn.patch
WARNING: A patch subject line should describe the change not the tool
that found it
#4:
Subject: [PATCH] smb: client: Fix checkpatch whitespace errors and warnings

total: 0 errors, 1 warnings, 107 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-smb-client-Fix-checkpatch-whitespace-errors-and-warn.patch has
style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

On Sun, Nov 26, 2023 at 10:52 PM Pierre Mariani
<pierre.mariani@gmail.com> wrote:
>
> Fixes no-op checkpatch errors and warnings.
>
> Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> ---
>  fs/smb/client/connect.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index a381c4cdb8c4..59f95ea5105e 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -482,6 +482,7 @@ static int reconnect_target_unlocked(struct TCP_Server_Info *server, struct dfs_
>  static int reconnect_dfs_server(struct TCP_Server_Info *server)
>  {
>         struct dfs_cache_tgt_iterator *target_hint = NULL;
> +
>         DFS_CACHE_TGT_LIST(tl);
>         int num_targets = 0;
>         int rc = 0;
> @@ -750,6 +751,7 @@ cifs_read_from_socket(struct TCP_Server_Info *server, char *buf,
>  {
>         struct msghdr smb_msg = {};
>         struct kvec iov = {.iov_base = buf, .iov_len = to_read};
> +
>         iov_iter_kvec(&smb_msg.msg_iter, ITER_DEST, &iov, 1, to_read);
>
>         return cifs_readv_from_socket(server, &smb_msg);
> @@ -1400,11 +1402,13 @@ cifs_match_ipaddr(struct sockaddr *srcaddr, struct sockaddr *rhs)
>         case AF_INET: {
>                 struct sockaddr_in *saddr4 = (struct sockaddr_in *)srcaddr;
>                 struct sockaddr_in *vaddr4 = (struct sockaddr_in *)rhs;
> +
>                 return (saddr4->sin_addr.s_addr == vaddr4->sin_addr.s_addr);
>         }
>         case AF_INET6: {
>                 struct sockaddr_in6 *saddr6 = (struct sockaddr_in6 *)srcaddr;
>                 struct sockaddr_in6 *vaddr6 = (struct sockaddr_in6 *)rhs;
> +
>                 return (ipv6_addr_equal(&saddr6->sin6_addr, &vaddr6->sin6_addr)
>                         && saddr6->sin6_scope_id == vaddr6->sin6_scope_id);
>         }
> @@ -2605,8 +2609,8 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
>                         rc = -EOPNOTSUPP;
>                         goto out_fail;
>                 } else {
> -                       cifs_dbg(VFS, "Check vers= mount option. SMB3.11 "
> -                               "disabled but required for POSIX extensions\n");
> +                       cifs_dbg(VFS,
> +                               "Check vers= mount option. SMB3.11 disabled but required for POSIX extensions\n");
>                         rc = -EOPNOTSUPP;
>                         goto out_fail;
>                 }
> @@ -2751,7 +2755,6 @@ cifs_put_tlink(struct tcon_link *tlink)
>         if (!IS_ERR(tlink_tcon(tlink)))
>                 cifs_put_tcon(tlink_tcon(tlink));
>         kfree(tlink);
> -       return;
>  }
>
>  static int
> @@ -2892,6 +2895,7 @@ static inline void
>  cifs_reclassify_socket4(struct socket *sock)
>  {
>         struct sock *sk = sock->sk;
> +
>         BUG_ON(!sock_allow_reclassification(sk));
>         sock_lock_init_class_and_name(sk, "slock-AF_INET-CIFS",
>                 &cifs_slock_key[0], "sk_lock-AF_INET-CIFS", &cifs_key[0]);
> @@ -2901,6 +2905,7 @@ static inline void
>  cifs_reclassify_socket6(struct socket *sock)
>  {
>         struct sock *sk = sock->sk;
> +
>         BUG_ON(!sock_allow_reclassification(sk));
>         sock_lock_init_class_and_name(sk, "slock-AF_INET6-CIFS",
>                 &cifs_slock_key[1], "sk_lock-AF_INET6-CIFS", &cifs_key[1]);
> @@ -2935,15 +2940,18 @@ static int
>  bind_socket(struct TCP_Server_Info *server)
>  {
>         int rc = 0;
> +
>         if (server->srcaddr.ss_family != AF_UNSPEC) {
>                 /* Bind to the specified local IP address */
>                 struct socket *socket = server->ssocket;
> +
>                 rc = kernel_bind(socket,
>                                  (struct sockaddr *) &server->srcaddr,
>                                  sizeof(server->srcaddr));
>                 if (rc < 0) {
>                         struct sockaddr_in *saddr4;
>                         struct sockaddr_in6 *saddr6;
> +
>                         saddr4 = (struct sockaddr_in *)&server->srcaddr;
>                         saddr6 = (struct sockaddr_in6 *)&server->srcaddr;
>                         if (saddr6->sin6_family == AF_INET6)
> @@ -3173,6 +3181,7 @@ void reset_cifs_unix_caps(unsigned int xid, struct cifs_tcon *tcon,
>
>         if (!CIFSSMBQFSUnixInfo(xid, tcon)) {
>                 __u64 cap = le64_to_cpu(tcon->fsUnixInfo.Capability);
> +
>                 cifs_dbg(FYI, "unix caps which server supports %lld\n", cap);
>                 /*
>                  * check for reconnect case in which we do not
> @@ -3676,7 +3685,7 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>         smb_buffer_response = smb_buffer;
>
>         header_assemble(smb_buffer, SMB_COM_TREE_CONNECT_ANDX,
> -                       NULL /*no tid */ , 4 /*wct */ );
> +                       NULL /*no tid */, 4 /*wct */);
>
>         smb_buffer->Mid = get_next_mid(ses->server);
>         smb_buffer->Uid = ses->Suid;
> @@ -3695,12 +3704,12 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses,
>         if (ses->server->sign)
>                 smb_buffer->Flags2 |= SMBFLG2_SECURITY_SIGNATURE;
>
> -       if (ses->capabilities & CAP_STATUS32) {
> +       if (ses->capabilities & CAP_STATUS32)
>                 smb_buffer->Flags2 |= SMBFLG2_ERR_STATUS;
> -       }
> -       if (ses->capabilities & CAP_DFS) {
> +
> +       if (ses->capabilities & CAP_DFS)
>                 smb_buffer->Flags2 |= SMBFLG2_DFS;
> -       }
> +
>         if (ses->capabilities & CAP_UNICODE) {
>                 smb_buffer->Flags2 |= SMBFLG2_UNICODE;
>                 length =
> --
> 2.39.2
>


-- 
Thanks,

Steve

Re: [PATCH 2/4] smb: client: Protect ses->chans update with chan_lock spin lock

[Pierre Mariani]

On Wed, Nov 29, 2023 at 04:30:54PM -0800, Pierre Mariani wrote:
> On Wed, Nov 29, 2023 at 02:30:37PM +0530, Shyam Prasad N wrote:
> > On Mon, Nov 27, 2023 at 10:22 AM Pierre Mariani
> > <pierre.mariani@gmail.com> wrote:
> > >
> > > Protect the update of ses->chans with chan_lock spin lock as per documentation
> > > from cifsglob.h.
> > > Fixes Coverity 1561738.
> > >
> > > Signed-off-by: Pierre Mariani <pierre.mariani@gmail.com>
> > > ---
> > >  fs/smb/client/connect.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > > index 449d56802692..0512835f399c 100644
> > > --- a/fs/smb/client/connect.c
> > > +++ b/fs/smb/client/connect.c
> > > @@ -2055,6 +2055,7 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> > >         spin_unlock(&cifs_tcp_ses_lock);
> > >
> > >         /* close any extra channels */
> > > +       spin_lock(&ses->chan_lock);
> > >         for (i = 1; i < ses->chan_count; i++) {
> > >                 if (ses->chans[i].iface) {
> > >                         kref_put(&ses->chans[i].iface->refcount, release_iface);
> > > @@ -2063,11 +2064,14 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> > >                 cifs_put_tcp_session(ses->chans[i].server, 0);
> > >                 ses->chans[i].server = NULL;
> > >         }
> > > +       spin_unlock(&ses->chan_lock);
> > >
> > >         /* we now account for primary channel in iface->refcount */
> > >         if (ses->chans[0].iface) {
> > >                 kref_put(&ses->chans[0].iface->refcount, release_iface);
> > > +               spin_lock(&ses->chan_lock);
> > >                 ses->chans[0].server = NULL;
> > > +               spin_unlock(&ses->chan_lock);
> > >         }
> > >
> > >         sesInfoFree(ses);
> > > --
> > > 2.39.2
> > >
> > >
> > 
> > Hi Pierre,
> > 
> > Thanks for proposing this change.
> > 
> > While it is true in general that chan_lock needs to be locked when
> > dealing with session channel details, this particular instance above
> > is during __cifs_put_smb_ses.
> > And this code is reached when ses_count has already reached 0. i.e.
> > this process is the last user of the session.
> > So taking chan_lock can be avoided. We did have this under a lock
> > before, but it resulted in deadlocks due to calls to
> > cifs_put_tcp_session, which locks bigger locks.
> > So the quick and dirty fix at that point was to not take chan_lock
> > here, knowing that we'll be the last user.
> > 
> > Perhaps a better fix exists?
> > Or we should probably document this as a comment for now.
> > 
> > This version of the patch will result in the deadlocks again.
> 
> Thank you for educating me on this, Shyam. I will re-read the code from that
> point of view and see if I can think of any improvement.
> 

Looking at the code in more details, I can see how the order of relevant locks
is cifs_tcp_ses_lock > srv_lock > chan_lock.
cifs_tcp_ses_lock and srv_lock are locked by cifs_put_tcp_session.
__cifs_put_smb_ses calls cifs_put_tcp_session. Hence we cannot lock chan_lock
and then call cifs_put_tcp_session or the lock order will not be respected.

To work around this issue, the following change could be made to read the value
of ses->chan_lock while holding chan_lock, but release it before starting the
loop where cifs_put_tcp_session is called.

-       for (i = 1; i < ses->chan_count; i++) {
+       spin_lock(&ses->chan_lock);
+       int ses_chan_count = ses->chan_count;
+
+       spin_unlock(&ses->chan_lock);
+
+       for (i = 1; i < ses_chan_count; i++) {

Please, let me know if this is acceptable.

> > 
> > -- 
> > Regards,
> > Shyam