[PATCH] cifs: wait for tcon resource_id before getting fscache super

[PATCH] cifs: wait for tcon resource_id before getting fscache super

[Shyam Prasad N]

The logic for initializing tcon->resource_id is done inside
cifs_root_iget. fscache super cookie relies on this for aux
data. So we need to push the fscache initialization to this
later point during mount.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/connect.c | 6 ------
 fs/cifs/fscache.c | 2 +-
 fs/cifs/inode.c   | 7 +++++++
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 6b705026da1a3..eee994b0925ff 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx)
  cifs_dbg(VFS, "read only mount of RW share\n");
  /* no need to log a RW mount of a typical RW share */
  }
- /*
- * The cookie is initialized from volume info returned above.
- * Inside cifs_fscache_get_super_cookie it checks
- * that we do not get super cookie twice.
- */
- cifs_fscache_get_super_cookie(tcon);
  }

  /*
diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
index 7e409a38a2d7c..f4da693760c11 100644
--- a/fs/cifs/fscache.c
+++ b/fs/cifs/fscache.c
@@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon)
  * In the future, as we integrate with newer fscache features,
  * we may want to instead add a check if cookie has changed
  */
- if (tcon->fscache == NULL)
+ if (tcon->fscache)
  return;

  sharename = extract_sharename(tcon->treeName);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 82848412ad852..96d083db17372 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
  inode = ERR_PTR(rc);
  }

+ /*
+ * The cookie is initialized from volume info returned above.
+ * Inside cifs_fscache_get_super_cookie it checks
+ * that we do not get super cookie twice.
+ */
+ cifs_fscache_get_super_cookie(tcon);
+
 out:
  kfree(path);
  free_xid(xid);

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[David Howells]

Your patches all got mangled.  Spaces converted to tabs.

David

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

These are what I had downloaded and tentatively merged into
cifs-2.6.git for-next

On Fri, Dec 3, 2021 at 3:23 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> The logic for initializing tcon->resource_id is done inside
> cifs_root_iget. fscache super cookie relies on this for aux
> data. So we need to push the fscache initialization to this
> later point during mount.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/cifs/connect.c | 6 ------
>  fs/cifs/fscache.c | 2 +-
>  fs/cifs/inode.c   | 7 +++++++
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 6b705026da1a3..eee994b0925ff 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx)
>   cifs_dbg(VFS, "read only mount of RW share\n");
>   /* no need to log a RW mount of a typical RW share */
>   }
> - /*
> - * The cookie is initialized from volume info returned above.
> - * Inside cifs_fscache_get_super_cookie it checks
> - * that we do not get super cookie twice.
> - */
> - cifs_fscache_get_super_cookie(tcon);
>   }
>
>   /*
> diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
> index 7e409a38a2d7c..f4da693760c11 100644
> --- a/fs/cifs/fscache.c
> +++ b/fs/cifs/fscache.c
> @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon)
>   * In the future, as we integrate with newer fscache features,
>   * we may want to instead add a check if cookie has changed
>   */
> - if (tcon->fscache == NULL)
> + if (tcon->fscache)
>   return;
>
>   sharename = extract_sharename(tcon->treeName);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 82848412ad852..96d083db17372 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
>   inode = ERR_PTR(rc);
>   }
>
> + /*
> + * The cookie is initialized from volume info returned above.
> + * Inside cifs_fscache_get_super_cookie it checks
> + * that we do not get super cookie twice.
> + */
> + cifs_fscache_get_super_cookie(tcon);
> +
>  out:
>   kfree(path);
>   free_xid(xid);



-- 
Thanks,

Steve

[-- Attachment #2: 1shyam-d70e50047c7daa3de17c7c41a0c4ef4f9e4443c9.patch --]
[-- Type: text/x-patch, Size: 2148 bytes --]

From d70e50047c7daa3de17c7c41a0c4ef4f9e4443c9 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Thu, 2 Dec 2021 07:14:42 +0000
Subject: [PATCH] cifs: wait for tcon resource_id before getting fscache super

The logic for initializing tcon->resource_id is done inside
cifs_root_iget. fscache super cookie relies on this for aux
data. So we need to push the fscache initialization to this
later point during mount.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/connect.c | 6 ------
 fs/cifs/fscache.c | 2 +-
 fs/cifs/inode.c   | 7 +++++++
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 6b705026da1a3..eee994b0925ff 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx)
 				cifs_dbg(VFS, "read only mount of RW share\n");
 			/* no need to log a RW mount of a typical RW share */
 		}
-		/*
-		 * The cookie is initialized from volume info returned above.
-		 * Inside cifs_fscache_get_super_cookie it checks
-		 * that we do not get super cookie twice.
-		 */
-		cifs_fscache_get_super_cookie(tcon);
 	}
 
 	/*
diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
index 7e409a38a2d7c..f4da693760c11 100644
--- a/fs/cifs/fscache.c
+++ b/fs/cifs/fscache.c
@@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon)
 	 * In the future, as we integrate with newer fscache features,
 	 * we may want to instead add a check if cookie has changed
 	 */
-	if (tcon->fscache == NULL)
+	if (tcon->fscache)
 		return;
 
 	sharename = extract_sharename(tcon->treeName);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 82848412ad852..96d083db17372 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		inode = ERR_PTR(rc);
 	}
 
+	/*
+	 * The cookie is initialized from volume info returned above.
+	 * Inside cifs_fscache_get_super_cookie it checks
+	 * that we do not get super cookie twice.
+	 */
+	cifs_fscache_get_super_cookie(tcon);
+
 out:
 	kfree(path);
 	free_xid(xid);

[-- Attachment #3: 2shyam-089dd629653b857b6096966e9d2df301653a36ea.patch --]
[-- Type: text/x-patch, Size: 2353 bytes --]

From 089dd629653b857b6096966e9d2df301653a36ea Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Thu, 2 Dec 2021 07:30:00 +0000
Subject: [PATCH] cifs: add server conn_id to fscache client cookie

The fscache client cookie uses the server address
(and port) as the cookie key. This is a problem when
nosharesock is used. Two different connections will
use duplicate cookies. Avoid this by adding
server->conn_id to the key, so that it's guaranteed
that cookie will not be duplicated.

Also, for secondary channels of a session, copy the
fscache pointer from the primary channel. The primary
channel is guaranteed not to go away as long as secondary
channels are in use.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/connect.c |  2 ++
 fs/cifs/fscache.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index eee994b0925f..d8822e835cc4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1562,6 +1562,8 @@ cifs_get_tcp_session(struct smb3_fs_context *ctx,
 	/* fscache server cookies are based on primary channel only */
 	if (!CIFS_SERVER_IS_CHAN(tcp_ses))
 		cifs_fscache_get_client_cookie(tcp_ses);
+	else
+		tcp_ses->fscache = tcp_ses->primary_server->fscache;
 
 	/* queue echo request delayed work */
 	queue_delayed_work(cifsiod_wq, &tcp_ses->echo, tcp_ses->echo_interval);
diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
index f4da693760c1..1db3437f3b7d 100644
--- a/fs/cifs/fscache.c
+++ b/fs/cifs/fscache.c
@@ -24,6 +24,7 @@ struct cifs_server_key {
 		struct in_addr	ipv4_addr;
 		struct in6_addr	ipv6_addr;
 	};
+	__u64 conn_id;
 } __packed;
 
 /*
@@ -37,6 +38,14 @@ void cifs_fscache_get_client_cookie(struct TCP_Server_Info *server)
 	struct cifs_server_key key;
 	uint16_t key_len = sizeof(key.hdr);
 
+	/*
+	 * Check if cookie was already initialized so don't reinitialize it.
+	 * In the future, as we integrate with newer fscache features,
+	 * we may want to instead add a check if cookie has changed
+	 */
+	if (server->fscache)
+		return;
+
 	memset(&key, 0, sizeof(key));
 
 	/*
@@ -62,6 +71,7 @@ void cifs_fscache_get_client_cookie(struct TCP_Server_Info *server)
 		server->fscache = NULL;
 		return;
 	}
+	key.conn_id = server->conn_id;
 
 	server->fscache =
 		fscache_acquire_cookie(cifs_fscache_netfs.primary_index,

[-- Attachment #4: 3shyam-a9a62cdfe26c782dadd0b94ab529b883426d0acd.patch --]
[-- Type: text/x-patch, Size: 2490 bytes --]

From a9a62cdfe26c782dadd0b94ab529b883426d0acd Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Thu, 2 Dec 2021 07:46:54 +0000
Subject: [PATCH] cifs: avoid use of dstaddr as key for fscache client cookie

server->dstaddr can change when the DNS mapping for the
server hostname changes. But conn_id is a u64 counter
that is incremented each time a new TCP connection
is setup. So use only that as a key.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/fscache.c | 38 +-------------------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
index 1db3437f3b7d..003c5f1f4dfb 100644
--- a/fs/cifs/fscache.c
+++ b/fs/cifs/fscache.c
@@ -16,14 +16,6 @@
  * Key layout of CIFS server cache index object
  */
 struct cifs_server_key {
-	struct {
-		uint16_t	family;		/* address family */
-		__be16		port;		/* IP port */
-	} hdr;
-	union {
-		struct in_addr	ipv4_addr;
-		struct in6_addr	ipv6_addr;
-	};
 	__u64 conn_id;
 } __packed;
 
@@ -32,11 +24,7 @@ struct cifs_server_key {
  */
 void cifs_fscache_get_client_cookie(struct TCP_Server_Info *server)
 {
-	const struct sockaddr *sa = (struct sockaddr *) &server->dstaddr;
-	const struct sockaddr_in *addr = (struct sockaddr_in *) sa;
-	const struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) sa;
 	struct cifs_server_key key;
-	uint16_t key_len = sizeof(key.hdr);
 
 	/*
 	 * Check if cookie was already initialized so don't reinitialize it.
@@ -47,36 +35,12 @@ void cifs_fscache_get_client_cookie(struct TCP_Server_Info *server)
 		return;
 
 	memset(&key, 0, sizeof(key));
-
-	/*
-	 * Should not be a problem as sin_family/sin6_family overlays
-	 * sa_family field
-	 */
-	key.hdr.family = sa->sa_family;
-	switch (sa->sa_family) {
-	case AF_INET:
-		key.hdr.port = addr->sin_port;
-		key.ipv4_addr = addr->sin_addr;
-		key_len += sizeof(key.ipv4_addr);
-		break;
-
-	case AF_INET6:
-		key.hdr.port = addr6->sin6_port;
-		key.ipv6_addr = addr6->sin6_addr;
-		key_len += sizeof(key.ipv6_addr);
-		break;
-
-	default:
-		cifs_dbg(VFS, "Unknown network family '%d'\n", sa->sa_family);
-		server->fscache = NULL;
-		return;
-	}
 	key.conn_id = server->conn_id;
 
 	server->fscache =
 		fscache_acquire_cookie(cifs_fscache_netfs.primary_index,
 				       &cifs_fscache_server_index_def,
-				       &key, key_len,
+				       &key, sizeof(key),
 				       NULL, 0,
 				       server, 0, true);
 	cifs_dbg(FYI, "%s: (0x%p/0x%p)\n",

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[Jeff Layton]

On Fri, 2021-12-03 at 14:52 +0530, Shyam Prasad N wrote:
> The logic for initializing tcon->resource_id is done inside
> cifs_root_iget. fscache super cookie relies on this for aux
> data. So we need to push the fscache initialization to this
> later point during mount.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/cifs/connect.c | 6 ------
>  fs/cifs/fscache.c | 2 +-
>  fs/cifs/inode.c   | 7 +++++++
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 6b705026da1a3..eee994b0925ff 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx)
>   cifs_dbg(VFS, "read only mount of RW share\n");
>   /* no need to log a RW mount of a typical RW share */
>   }
> - /*
> - * The cookie is initialized from volume info returned above.
> - * Inside cifs_fscache_get_super_cookie it checks
> - * that we do not get super cookie twice.
> - */
> - cifs_fscache_get_super_cookie(tcon);
>   }
> 
>   /*
> diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
> index 7e409a38a2d7c..f4da693760c11 100644
> --- a/fs/cifs/fscache.c
> +++ b/fs/cifs/fscache.c
> @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon)
>   * In the future, as we integrate with newer fscache features,
>   * we may want to instead add a check if cookie has changed
>   */
> - if (tcon->fscache == NULL)
> + if (tcon->fscache)
>   return;
> 

Ouch! Does the above mean that fscache on cifs is just plain broken at
the moment? If this is the routine that sets the tcon cookie, then it
looks like it just never gets set?

>   sharename = extract_sharename(tcon->treeName);
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 82848412ad852..96d083db17372 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
>   inode = ERR_PTR(rc);
>   }
> 
> + /*
> + * The cookie is initialized from volume info returned above.
> + * Inside cifs_fscache_get_super_cookie it checks
> + * that we do not get super cookie twice.
> + */
> + cifs_fscache_get_super_cookie(tcon);
> +
>  out:
>   kfree(path);
>   free_xid(xid);
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[Paulo Alcantara]

On December 3, 2021 1:21:15 PM GMT-03:00, Jeff Layton <jlayton@redhat.com> wrote:
>On Fri, 2021-12-03 at 14:52 +0530, Shyam Prasad N wrote:
>> The logic for initializing tcon->resource_id is done inside
>> cifs_root_iget. fscache super cookie relies on this for aux
>> data. So we need to push the fscache initialization to this
>> later point during mount.
>> 
>> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
>> ---
>>  fs/cifs/connect.c | 6 ------
>>  fs/cifs/fscache.c | 2 +-
>>  fs/cifs/inode.c   | 7 +++++++
>>  3 files changed, 8 insertions(+), 7 deletions(-)
>> 
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 6b705026da1a3..eee994b0925ff 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -3046,12 +3046,6 @@ static int mount_get_conns(struct mount_ctx *mnt_ctx)
>>   cifs_dbg(VFS, "read only mount of RW share\n");
>>   /* no need to log a RW mount of a typical RW share */
>>   }
>> - /*
>> - * The cookie is initialized from volume info returned above.
>> - * Inside cifs_fscache_get_super_cookie it checks
>> - * that we do not get super cookie twice.
>> - */
>> - cifs_fscache_get_super_cookie(tcon);
>>   }
>> 
>>   /*
>> diff --git a/fs/cifs/fscache.c b/fs/cifs/fscache.c
>> index 7e409a38a2d7c..f4da693760c11 100644
>> --- a/fs/cifs/fscache.c
>> +++ b/fs/cifs/fscache.c
>> @@ -92,7 +92,7 @@ void cifs_fscache_get_super_cookie(struct cifs_tcon *tcon)
>>   * In the future, as we integrate with newer fscache features,
>>   * we may want to instead add a check if cookie has changed
>>   */
>> - if (tcon->fscache == NULL)
>> + if (tcon->fscache)
>>   return;
>> 
>
>Ouch! Does the above mean that fscache on cifs is just plain broken at
>the moment? If this is the routine that sets the tcon cookie, then it
>looks like it just never gets set?

Dont much know about fscache, but remember that multiple mounts can share a single tcon (if not using nosharesock).  So, if we find an existing tcon and we have a cookie for it already, the check makes sense.

>
>>   sharename = extract_sharename(tcon->treeName);
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 82848412ad852..96d083db17372 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
>>   inode = ERR_PTR(rc);
>>   }
>> 
>> + /*
>> + * The cookie is initialized from volume info returned above.
>> + * Inside cifs_fscache_get_super_cookie it checks
>> + * that we do not get super cookie twice.
>> + */
>> + cifs_fscache_get_super_cookie(tcon);
>> +
>>  out:
>>   kfree(path);
>>   free_xid(xid);
>> 
>

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[David Howells]

Shyam Prasad N <nspmangalore@gmail.com> wrote:

> @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
>   inode = ERR_PTR(rc);
>   }
> 
> + /*
> + * The cookie is initialized from volume info returned above.
> + * Inside cifs_fscache_get_super_cookie it checks
> + * that we do not get super cookie twice.
> + */
> + cifs_fscache_get_super_cookie(tcon);

Ummm...  Does this handle the errors correctly?  What happens if rc != 0 at
this point and the inode has been marked failed?  It looks like it will
abandon creation of the superblock without cleaning up the super cookie.
Maybe - or maybe it can't happen because of the:

	iget_no_retry:
		if (!inode) {
			inode = ERR_PTR(rc);
			goto out;
		}

check - but then why is rc being checked?

> +
>  out:
>   kfree(path);
>   free_xid(xid);

David

Re: [PATCH] cifs: wait for tcon resource_id before getting fscache super

[Shyam Prasad N]

On Mon, Dec 6, 2021 at 7:22 PM David Howells <dhowells@redhat.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> > @@ -1376,6 +1376,13 @@ struct inode *cifs_root_iget(struct super_block *sb)
> >   inode = ERR_PTR(rc);
> >   }
> >
> > + /*
> > + * The cookie is initialized from volume info returned above.
> > + * Inside cifs_fscache_get_super_cookie it checks
> > + * that we do not get super cookie twice.
> > + */
> > + cifs_fscache_get_super_cookie(tcon);
>
> Ummm...  Does this handle the errors correctly?  What happens if rc != 0 at
> this point and the inode has been marked failed?  It looks like it will
> abandon creation of the superblock without cleaning up the super cookie.
> Maybe - or maybe it can't happen because of the:
>
>         iget_no_retry:
>                 if (!inode) {
>                         inode = ERR_PTR(rc);
>                         goto out;
>                 }
>
> check - but then why is rc being checked?
>
> > +
> >  out:
> >   kfree(path);
> >   free_xid(xid);
>
> David
>

Thanks David. I think that there still needs to be more error handling here.
I'll check on this and send out another patch.

-- 
Regards,
Shyam