[PATCH 1/3] cifs: Delete a stray unlock in cifs_swn_reconnect()

[PATCH 1/3] cifs: Delete a stray unlock in cifs_swn_reconnect()

[Dan Carpenter]

The unlock is done in the caller, this is a stray which leads to a
double unlock bug.

Fixes: bf80e5d4259a ("cifs: Send witness register and unregister commands to userspace daemon")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/cifs/cifs_swn.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/cifs/cifs_swn.c b/fs/cifs/cifs_swn.c
index c594e588a8b5..b2ef082d6438 100644
--- a/fs/cifs/cifs_swn.c
+++ b/fs/cifs/cifs_swn.c
@@ -285,8 +285,6 @@ static struct cifs_swn_reg *cifs_find_swn_reg(struct cifs_tcon *tcon)
 			continue;
 		}
 
-		mutex_unlock(&cifs_swnreg_idr_mutex);
-
 		cifs_dbg(FYI, "Existing swn registration for %s:%s found\n", swnreg->net_name,
 				swnreg->share_name);
 
-- 
2.29.2

[PATCH 2/2] cifs: Unlock on errors in cifs_swn_reconnect()

[Dan Carpenter]

There are three error paths which need to unlock before returning.

Fixes: 121d947d4fe1 ("cifs: Handle witness client move notification")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/cifs/cifs_swn.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifs_swn.c b/fs/cifs/cifs_swn.c
index b2ef082d6438..91163d3cf8b7 100644
--- a/fs/cifs/cifs_swn.c
+++ b/fs/cifs/cifs_swn.c
@@ -480,16 +480,16 @@ static int cifs_swn_store_swn_addr(const struct sockaddr_storage *new,
 
 static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *addr)
 {
+	int ret = 0;
+
 	/* Store the reconnect address */
 	mutex_lock(&tcon->ses->server->srv_mutex);
 	if (!cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr)) {
-		int ret;
-
 		ret = cifs_swn_store_swn_addr(addr, &tcon->ses->server->dstaddr,
 				&tcon->ses->server->swn_dstaddr);
 		if (ret < 0) {
 			cifs_dbg(VFS, "%s: failed to store address: %d\n", __func__, ret);
-			return ret;
+			goto unlock;
 		}
 		tcon->ses->server->use_swn_dstaddr = true;
 
@@ -500,7 +500,7 @@ static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *a
 		if (ret < 0) {
 			cifs_dbg(VFS, "%s: Failed to unregister for witness notifications: %d\n",
 					__func__, ret);
-			return ret;
+			goto unlock;
 		}
 
 		/*
@@ -511,7 +511,7 @@ static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *a
 		if (ret < 0) {
 			cifs_dbg(VFS, "%s: Failed to register for witness notifications: %d\n",
 					__func__, ret);
-			return ret;
+			goto unlock;
 		}
 
 		spin_lock(&GlobalMid_Lock);
@@ -519,9 +519,10 @@ static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *a
 			tcon->ses->server->tcpStatus = CifsNeedReconnect;
 		spin_unlock(&GlobalMid_Lock);
 	}
+unlock:
 	mutex_unlock(&tcon->ses->server->srv_mutex);
 
-	return 0;
+	return ret;
 }
 
 static int cifs_swn_client_move(struct cifs_swn_reg *swnreg, struct sockaddr_storage *addr)
-- 
2.29.2

Re: [PATCH 2/2] cifs: Unlock on errors in cifs_swn_reconnect()

[Samuel Cabrero]

On Thu, 2020-12-17 at 14:02 +0300, Dan Carpenter wrote:
> There are three error paths which need to unlock before returning.

Thanks.

Reviewed-by: Samuel Cabrero <scabrero@suse.de>

-- 
Samuel Cabrero / SUSE Labs Samba Team
GPG: D7D6 E259 F91C F0B3 2E61 1239 3655 6EC9 7051 0856
scabrero@suse.com
scabrero@suse.de

[PATCH 3/3] cifs: Re-indent cifs_swn_reconnect()

[Dan Carpenter]

This code is slightly nicer if we flip the cifs_sockaddr_equal()
around and pull all the code in one tab.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/cifs/cifs_swn.c | 64 ++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/fs/cifs/cifs_swn.c b/fs/cifs/cifs_swn.c
index 91163d3cf8b7..d35f599aa00e 100644
--- a/fs/cifs/cifs_swn.c
+++ b/fs/cifs/cifs_swn.c
@@ -484,41 +484,43 @@ static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *a
 
 	/* Store the reconnect address */
 	mutex_lock(&tcon->ses->server->srv_mutex);
-	if (!cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr)) {
-		ret = cifs_swn_store_swn_addr(addr, &tcon->ses->server->dstaddr,
-				&tcon->ses->server->swn_dstaddr);
-		if (ret < 0) {
-			cifs_dbg(VFS, "%s: failed to store address: %d\n", __func__, ret);
-			goto unlock;
-		}
-		tcon->ses->server->use_swn_dstaddr = true;
+	if (cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr))
+		goto unlock;
 
-		/*
-		 * Unregister to stop receiving notifications for the old IP address.
-		 */
-		ret = cifs_swn_unregister(tcon);
-		if (ret < 0) {
-			cifs_dbg(VFS, "%s: Failed to unregister for witness notifications: %d\n",
-					__func__, ret);
-			goto unlock;
-		}
+	ret = cifs_swn_store_swn_addr(addr, &tcon->ses->server->dstaddr,
+				      &tcon->ses->server->swn_dstaddr);
+	if (ret < 0) {
+		cifs_dbg(VFS, "%s: failed to store address: %d\n", __func__, ret);
+		goto unlock;
+	}
+	tcon->ses->server->use_swn_dstaddr = true;
 
-		/*
-		 * And register to receive notifications for the new IP address now that we have
-		 * stored the new address.
-		 */
-		ret = cifs_swn_register(tcon);
-		if (ret < 0) {
-			cifs_dbg(VFS, "%s: Failed to register for witness notifications: %d\n",
-					__func__, ret);
-			goto unlock;
-		}
+	/*
+	 * Unregister to stop receiving notifications for the old IP address.
+	 */
+	ret = cifs_swn_unregister(tcon);
+	if (ret < 0) {
+		cifs_dbg(VFS, "%s: Failed to unregister for witness notifications: %d\n",
+			 __func__, ret);
+		goto unlock;
+	}
 
-		spin_lock(&GlobalMid_Lock);
-		if (tcon->ses->server->tcpStatus != CifsExiting)
-			tcon->ses->server->tcpStatus = CifsNeedReconnect;
-		spin_unlock(&GlobalMid_Lock);
+	/*
+	 * And register to receive notifications for the new IP address now that we have
+	 * stored the new address.
+	 */
+	ret = cifs_swn_register(tcon);
+	if (ret < 0) {
+		cifs_dbg(VFS, "%s: Failed to register for witness notifications: %d\n",
+			 __func__, ret);
+		goto unlock;
 	}
+
+	spin_lock(&GlobalMid_Lock);
+	if (tcon->ses->server->tcpStatus != CifsExiting)
+		tcon->ses->server->tcpStatus = CifsNeedReconnect;
+	spin_unlock(&GlobalMid_Lock);
+
 unlock:
 	mutex_unlock(&tcon->ses->server->srv_mutex);
 
-- 
2.29.2

Re: [PATCH 3/3] cifs: Re-indent cifs_swn_reconnect()

[Samuel Cabrero]

On Thu, 2020-12-17 at 14:03 +0300, Dan Carpenter wrote:
> This code is slightly nicer if we flip the cifs_sockaddr_equal()
> around and pull all the code in one tab.
> 

I Agree.

Reviewed-by: Samuel Cabrero <scabrero@suse.de>

-- 
Samuel Cabrero / SUSE Labs Samba Team
GPG: D7D6 E259 F91C F0B3 2E61 1239 3655 6EC9 7051 0856
scabrero@suse.com
scabrero@suse.de

Re: [PATCH 3/3] cifs: Re-indent cifs_swn_reconnect()

[Steve French]

tentatively merged these three into cifs-2.6.git for-next pending testing

On Thu, Dec 17, 2020 at 5:04 AM Dan Carpenter via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> This code is slightly nicer if we flip the cifs_sockaddr_equal()
> around and pull all the code in one tab.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  fs/cifs/cifs_swn.c | 64 ++++++++++++++++++++++++----------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/fs/cifs/cifs_swn.c b/fs/cifs/cifs_swn.c
> index 91163d3cf8b7..d35f599aa00e 100644
> --- a/fs/cifs/cifs_swn.c
> +++ b/fs/cifs/cifs_swn.c
> @@ -484,41 +484,43 @@ static int cifs_swn_reconnect(struct cifs_tcon *tcon, struct sockaddr_storage *a
>
>         /* Store the reconnect address */
>         mutex_lock(&tcon->ses->server->srv_mutex);
> -       if (!cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr)) {
> -               ret = cifs_swn_store_swn_addr(addr, &tcon->ses->server->dstaddr,
> -                               &tcon->ses->server->swn_dstaddr);
> -               if (ret < 0) {
> -                       cifs_dbg(VFS, "%s: failed to store address: %d\n", __func__, ret);
> -                       goto unlock;
> -               }
> -               tcon->ses->server->use_swn_dstaddr = true;
> +       if (cifs_sockaddr_equal(&tcon->ses->server->dstaddr, addr))
> +               goto unlock;
>
> -               /*
> -                * Unregister to stop receiving notifications for the old IP address.
> -                */
> -               ret = cifs_swn_unregister(tcon);
> -               if (ret < 0) {
> -                       cifs_dbg(VFS, "%s: Failed to unregister for witness notifications: %d\n",
> -                                       __func__, ret);
> -                       goto unlock;
> -               }
> +       ret = cifs_swn_store_swn_addr(addr, &tcon->ses->server->dstaddr,
> +                                     &tcon->ses->server->swn_dstaddr);
> +       if (ret < 0) {
> +               cifs_dbg(VFS, "%s: failed to store address: %d\n", __func__, ret);
> +               goto unlock;
> +       }
> +       tcon->ses->server->use_swn_dstaddr = true;
>
> -               /*
> -                * And register to receive notifications for the new IP address now that we have
> -                * stored the new address.
> -                */
> -               ret = cifs_swn_register(tcon);
> -               if (ret < 0) {
> -                       cifs_dbg(VFS, "%s: Failed to register for witness notifications: %d\n",
> -                                       __func__, ret);
> -                       goto unlock;
> -               }
> +       /*
> +        * Unregister to stop receiving notifications for the old IP address.
> +        */
> +       ret = cifs_swn_unregister(tcon);
> +       if (ret < 0) {
> +               cifs_dbg(VFS, "%s: Failed to unregister for witness notifications: %d\n",
> +                        __func__, ret);
> +               goto unlock;
> +       }
>
> -               spin_lock(&GlobalMid_Lock);
> -               if (tcon->ses->server->tcpStatus != CifsExiting)
> -                       tcon->ses->server->tcpStatus = CifsNeedReconnect;
> -               spin_unlock(&GlobalMid_Lock);
> +       /*
> +        * And register to receive notifications for the new IP address now that we have
> +        * stored the new address.
> +        */
> +       ret = cifs_swn_register(tcon);
> +       if (ret < 0) {
> +               cifs_dbg(VFS, "%s: Failed to register for witness notifications: %d\n",
> +                        __func__, ret);
> +               goto unlock;
>         }
> +
> +       spin_lock(&GlobalMid_Lock);
> +       if (tcon->ses->server->tcpStatus != CifsExiting)
> +               tcon->ses->server->tcpStatus = CifsNeedReconnect;
> +       spin_unlock(&GlobalMid_Lock);
> +
>  unlock:
>         mutex_unlock(&tcon->ses->server->srv_mutex);
>
> --
> 2.29.2
>
>


-- 
Thanks,

Steve

Re: [PATCH 1/3] cifs: Delete a stray unlock in cifs_swn_reconnect()

[Samuel Cabrero]

On Thu, 2020-12-17 at 14:01 +0300, Dan Carpenter wrote:
> The unlock is done in the caller, this is a stray which leads to a
> double unlock bug.
> 

Indeed.

Reviewed-by: Samuel Cabrero <scabrero@suse.de>

-- 
Samuel Cabrero / SUSE Labs Samba Team
GPG: D7D6 E259 F91C F0B3 2E61 1239 3655 6EC9 7051 0856
scabrero@suse.com
scabrero@suse.de