[PATCH 0/5] afs: Miscellaneous small fixes

[PATCH 0/5] afs: Miscellaneous small fixes

[David Howells]

Hi Marc,

Here are a set of miscellaneous small fixes to the afs filesystem
including:

 (1) Fix the afs_server_list struct to be cleaned up with RCU.

 (2) Fix afs to translate a no-data result from a DNS lookup into ENOENT,
     not EDESTADDRREQ for consistency with OpenAFS.

 (3) Fix afs to translate a negative DNS lookup result into ENOENT rather
     than EDESTADDRREQ.

 (4) Fix file locking on R/O volumes to operate in local mode as the server
     doesn't handle exclusive locks on such files.

 (5) Not a fix per se, but set SB_RDONLY on superblocks for RO and Backup
     volumes so that the VFS can see that they're read only.

The patches can be found here:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes

Thanks,
David

David Howells (5):
  afs: Fix afs_server_list to be cleaned up with RCU
  afs: Make error on cell lookup failure consistent with OpenAFS
  afs: Return ENOENT if no cell DNS record can be found
  afs: Fix file locking on R/O volumes to operate in local mode
  afs: Mark a superblock for an R/O or Backup volume as SB_RDONLY

 fs/afs/dynroot.c     |  4 ++--
 fs/afs/internal.h    |  1 +
 fs/afs/server_list.c |  2 +-
 fs/afs/super.c       |  4 ++++
 fs/afs/vl_rotate.c   | 10 ++++++++++
 5 files changed, 18 insertions(+), 3 deletions(-)

[PATCH 1/5] afs: Fix afs_server_list to be cleaned up with RCU

[David Howells]

afs_server_list is accessed with the rcu_read_lock() held from
volume->servers, so it needs to be cleaned up correctly.

Fix this by using kfree_rcu() instead of kfree().

Fixes: 8a070a964877 ("afs: Detect cell aliases 1 - Cells with root volumes")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
 fs/afs/internal.h    | 1 +
 fs/afs/server_list.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index c9cef3782b4a..a812952be1c9 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -553,6 +553,7 @@ struct afs_server_entry {
 };
 
 struct afs_server_list {
+	struct rcu_head		rcu;
 	afs_volid_t		vids[AFS_MAXTYPES]; /* Volume IDs */
 	refcount_t		usage;
 	unsigned char		nr_servers;
diff --git a/fs/afs/server_list.c b/fs/afs/server_list.c
index ed9056703505..b59896b1de0a 100644
--- a/fs/afs/server_list.c
+++ b/fs/afs/server_list.c
@@ -17,7 +17,7 @@ void afs_put_serverlist(struct afs_net *net, struct afs_server_list *slist)
 		for (i = 0; i < slist->nr_servers; i++)
 			afs_unuse_server(net, slist->servers[i].server,
 					 afs_server_trace_put_slist);
-		kfree(slist);
+		kfree_rcu(slist, rcu);
 	}
 }
 

[PATCH 2/5] afs: Make error on cell lookup failure consistent with OpenAFS

[David Howells]

When kafs tries to look up a cell in the DNS or the local config, it will
translate a lookup failure into EDESTADDRREQ whereas OpenAFS translates it
into ENOENT.  Applications such as West expect the latter behaviour and
fail if they see the former.

This can be seen by trying to mount an unknown cell:

   # mount -t afs %example.com:cell.root /mnt
   mount: /mnt: mount(2) system call failed: Destination address required.

Fixes: 4d673da14533 ("afs: Support the AFS dynamic root")
Reported-by: Markus Suvanto <markus.suvanto@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
 fs/afs/dynroot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index 4d04ef2d3ae7..1fa8cf23bd36 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -132,8 +132,8 @@ static int afs_probe_cell_name(struct dentry *dentry)
 
 	ret = dns_query(net->net, "afsdb", name, len, "srv=1",
 			NULL, NULL, false);
-	if (ret == -ENODATA)
-		ret = -EDESTADDRREQ;
+	if (ret == -ENODATA || ret == -ENOKEY)
+		ret = -ENOENT;
 	return ret;
 }
 

[PATCH 3/5] afs: Return ENOENT if no cell DNS record can be found

[David Howells]

Make AFS return error ENOENT if no cell SRV or AFSDB DNS record (or
cellservdb config file record) can be found rather than returning
EDESTADDRREQ.

Also add cell name lookup info to the cursor dump.

Fixes: d5c32c89b208 ("afs: Fix cell DNS lookup")
Reported-by: Markus Suvanto <markus.suvanto@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
 fs/afs/vl_rotate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/afs/vl_rotate.c b/fs/afs/vl_rotate.c
index 488e58490b16..eb415ce56360 100644
--- a/fs/afs/vl_rotate.c
+++ b/fs/afs/vl_rotate.c
@@ -58,6 +58,12 @@ static bool afs_start_vl_iteration(struct afs_vl_cursor *vc)
 		}
 
 		/* Status load is ordered after lookup counter load */
+		if (cell->dns_status == DNS_LOOKUP_GOT_NOT_FOUND) {
+			pr_warn("No record of cell %s\n", cell->name);
+			vc->error = -ENOENT;
+			return false;
+		}
+
 		if (cell->dns_source == DNS_RECORD_UNAVAILABLE) {
 			vc->error = -EDESTADDRREQ;
 			return false;
@@ -285,6 +291,7 @@ bool afs_select_vlserver(struct afs_vl_cursor *vc)
  */
 static void afs_vl_dump_edestaddrreq(const struct afs_vl_cursor *vc)
 {
+	struct afs_cell *cell = vc->cell;
 	static int count;
 	int i;
 
@@ -294,6 +301,9 @@ static void afs_vl_dump_edestaddrreq(const struct afs_vl_cursor *vc)
 
 	rcu_read_lock();
 	pr_notice("EDESTADDR occurred\n");
+	pr_notice("CELL: %s err=%d\n", cell->name, cell->error);
+	pr_notice("DNS: src=%u st=%u lc=%x\n",
+		  cell->dns_source, cell->dns_status, cell->dns_lookup_count);
 	pr_notice("VC: ut=%lx ix=%u ni=%hu fl=%hx err=%hd\n",
 		  vc->untried, vc->index, vc->nr_iterations, vc->flags, vc->error);
 

[PATCH 4/5] afs: Fix file locking on R/O volumes to operate in local mode

[David Howells]

AFS doesn't really do locking on R/O volumes as fileservers don't maintain
state with each other and thus a lock on a R/O volume file on one
fileserver will not be be visible to someone looking at the same file on
another fileserver.

Further, the server may return an error if you try it.

Fix this by doing what other AFS clients do and handle filelocking on R/O
volume files entirely within the client and don't touch the server.

Fixes: 6c6c1d63c243 ("afs: Provide mount-time configurable byte-range file locking emulation")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
 fs/afs/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/afs/super.c b/fs/afs/super.c
index 95d713074dc8..e95fb4cb4fcd 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -407,6 +407,8 @@ static int afs_validate_fc(struct fs_context *fc)
 			return PTR_ERR(volume);
 
 		ctx->volume = volume;
+		if (volume->type != AFSVL_RWVOL)
+			ctx->flock_mode = afs_flock_mode_local;
 	}
 
 	return 0;

[PATCH 5/5] afs: Mark a superblock for an R/O or Backup volume as SB_RDONLY

[David Howells]

Mark a superblock that is for for an R/O or Backup volume as SB_RDONLY when
mounting it.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
---
 fs/afs/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/afs/super.c b/fs/afs/super.c
index e95fb4cb4fcd..a01a0fb2cdbb 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -407,8 +407,10 @@ static int afs_validate_fc(struct fs_context *fc)
 			return PTR_ERR(volume);
 
 		ctx->volume = volume;
-		if (volume->type != AFSVL_RWVOL)
+		if (volume->type != AFSVL_RWVOL) {
 			ctx->flock_mode = afs_flock_mode_local;
+			fc->sb_flags |= SB_RDONLY;
+		}
 	}
 
 	return 0;

Re: [PATCH 5/5] afs: Mark a superblock for an R/O or Backup volume as SB_RDONLY

[Marc Dionne]

On Thu, Nov 16, 2023 at 11:53 AM David Howells <dhowells@redhat.com> wrote:
>
> Mark a superblock that is for for an R/O or Backup volume as SB_RDONLY when
> mounting it.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/super.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/afs/super.c b/fs/afs/super.c
> index e95fb4cb4fcd..a01a0fb2cdbb 100644
> --- a/fs/afs/super.c
> +++ b/fs/afs/super.c
> @@ -407,8 +407,10 @@ static int afs_validate_fc(struct fs_context *fc)
>                         return PTR_ERR(volume);
>
>                 ctx->volume = volume;
> -               if (volume->type != AFSVL_RWVOL)
> +               if (volume->type != AFSVL_RWVOL) {
>                         ctx->flock_mode = afs_flock_mode_local;
> +                       fc->sb_flags |= SB_RDONLY;
> +               }
>         }
>
>         return 0;

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc

Re: [PATCH 1/5] afs: Fix afs_server_list to be cleaned up with RCU

[Marc Dionne]

On Thu, Nov 16, 2023 at 11:53 AM David Howells <dhowells@redhat.com> wrote:
>
> afs_server_list is accessed with the rcu_read_lock() held from
> volume->servers, so it needs to be cleaned up correctly.
>
> Fix this by using kfree_rcu() instead of kfree().
>
> Fixes: 8a070a964877 ("afs: Detect cell aliases 1 - Cells with root volumes")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/internal.h    | 1 +
>  fs/afs/server_list.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index c9cef3782b4a..a812952be1c9 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -553,6 +553,7 @@ struct afs_server_entry {
>  };
>
>  struct afs_server_list {
> +       struct rcu_head         rcu;
>         afs_volid_t             vids[AFS_MAXTYPES]; /* Volume IDs */
>         refcount_t              usage;
>         unsigned char           nr_servers;
> diff --git a/fs/afs/server_list.c b/fs/afs/server_list.c
> index ed9056703505..b59896b1de0a 100644
> --- a/fs/afs/server_list.c
> +++ b/fs/afs/server_list.c
> @@ -17,7 +17,7 @@ void afs_put_serverlist(struct afs_net *net, struct afs_server_list *slist)
>                 for (i = 0; i < slist->nr_servers; i++)
>                         afs_unuse_server(net, slist->servers[i].server,
>                                          afs_server_trace_put_slist);
> -               kfree(slist);
> +               kfree_rcu(slist, rcu);
>         }
>  }

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc

Re: [PATCH 2/5] afs: Make error on cell lookup failure consistent with OpenAFS

[Marc Dionne]

On Thu, Nov 16, 2023 at 11:53 AM David Howells <dhowells@redhat.com> wrote:
>
> When kafs tries to look up a cell in the DNS or the local config, it will
> translate a lookup failure into EDESTADDRREQ whereas OpenAFS translates it
> into ENOENT.  Applications such as West expect the latter behaviour and
> fail if they see the former.
>
> This can be seen by trying to mount an unknown cell:
>
>    # mount -t afs %example.com:cell.root /mnt
>    mount: /mnt: mount(2) system call failed: Destination address required.
>
> Fixes: 4d673da14533 ("afs: Support the AFS dynamic root")
> Reported-by: Markus Suvanto <markus.suvanto@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/dynroot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
> index 4d04ef2d3ae7..1fa8cf23bd36 100644
> --- a/fs/afs/dynroot.c
> +++ b/fs/afs/dynroot.c
> @@ -132,8 +132,8 @@ static int afs_probe_cell_name(struct dentry *dentry)
>
>         ret = dns_query(net->net, "afsdb", name, len, "srv=1",
>                         NULL, NULL, false);
> -       if (ret == -ENODATA)
> -               ret = -EDESTADDRREQ;
> +       if (ret == -ENODATA || ret == -ENOKEY)
> +               ret = -ENOENT;
>         return ret;
>  }

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc

Re: [PATCH 0/5] afs: Miscellaneous small fixes

[Jeffrey E Altman]

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

On 11/16/2023 10:53 AM, David Howells wrote:
> Hi Marc,
>
> Here are a set of miscellaneous small fixes to the afs filesystem
> including:
>
>   (1) Fix the afs_server_list struct to be cleaned up with RCU.
>
>   (2) Fix afs to translate a no-data result from a DNS lookup into ENOENT,
>       not EDESTADDRREQ for consistency with OpenAFS.
>
>   (3) Fix afs to translate a negative DNS lookup result into ENOENT rather
>       than EDESTADDRREQ.
>
>   (4) Fix file locking on R/O volumes to operate in local mode as the server
>       doesn't handle exclusive locks on such files.
>
>   (5) Not a fix per se, but set SB_RDONLY on superblocks for RO and Backup
>       volumes so that the VFS can see that they're read only.
>
> The patches can be found here:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes
>
> Thanks,
> David
>
> David Howells (5):
>    afs: Fix afs_server_list to be cleaned up with RCU
>    afs: Make error on cell lookup failure consistent with OpenAFS
>    afs: Return ENOENT if no cell DNS record can be found
>    afs: Fix file locking on R/O volumes to operate in local mode
>    afs: Mark a superblock for an R/O or Backup volume as SB_RDONLY
>
>   fs/afs/dynroot.c     |  4 ++--
>   fs/afs/internal.h    |  1 +
>   fs/afs/server_list.c |  2 +-
>   fs/afs/super.c       |  4 ++++
>   fs/afs/vl_rotate.c   | 10 ++++++++++
>   5 files changed, 18 insertions(+), 3 deletions(-)

Reviewed-by: Jeffrey Altman <jaltman@auristor.com>


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

Re: [PATCH 3/5] afs: Return ENOENT if no cell DNS record can be found

[Marc Dionne]

On Thu, Nov 16, 2023 at 11:53 AM David Howells <dhowells@redhat.com> wrote:
>
> Make AFS return error ENOENT if no cell SRV or AFSDB DNS record (or
> cellservdb config file record) can be found rather than returning
> EDESTADDRREQ.
>
> Also add cell name lookup info to the cursor dump.
>
> Fixes: d5c32c89b208 ("afs: Fix cell DNS lookup")
> Reported-by: Markus Suvanto <markus.suvanto@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216637
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/vl_rotate.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/afs/vl_rotate.c b/fs/afs/vl_rotate.c
> index 488e58490b16..eb415ce56360 100644
> --- a/fs/afs/vl_rotate.c
> +++ b/fs/afs/vl_rotate.c
> @@ -58,6 +58,12 @@ static bool afs_start_vl_iteration(struct afs_vl_cursor *vc)
>                 }
>
>                 /* Status load is ordered after lookup counter load */
> +               if (cell->dns_status == DNS_LOOKUP_GOT_NOT_FOUND) {
> +                       pr_warn("No record of cell %s\n", cell->name);
> +                       vc->error = -ENOENT;
> +                       return false;
> +               }
> +
>                 if (cell->dns_source == DNS_RECORD_UNAVAILABLE) {
>                         vc->error = -EDESTADDRREQ;
>                         return false;
> @@ -285,6 +291,7 @@ bool afs_select_vlserver(struct afs_vl_cursor *vc)
>   */
>  static void afs_vl_dump_edestaddrreq(const struct afs_vl_cursor *vc)
>  {
> +       struct afs_cell *cell = vc->cell;
>         static int count;
>         int i;
>
> @@ -294,6 +301,9 @@ static void afs_vl_dump_edestaddrreq(const struct afs_vl_cursor *vc)
>
>         rcu_read_lock();
>         pr_notice("EDESTADDR occurred\n");
> +       pr_notice("CELL: %s err=%d\n", cell->name, cell->error);
> +       pr_notice("DNS: src=%u st=%u lc=%x\n",
> +                 cell->dns_source, cell->dns_status, cell->dns_lookup_count);
>         pr_notice("VC: ut=%lx ix=%u ni=%hu fl=%hx err=%hd\n",
>                   vc->untried, vc->index, vc->nr_iterations, vc->flags, vc->error);

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc

Re: [PATCH 4/5] afs: Fix file locking on R/O volumes to operate in local mode

[Marc Dionne]

On Thu, Nov 16, 2023 at 11:53 AM David Howells <dhowells@redhat.com> wrote:
>
> AFS doesn't really do locking on R/O volumes as fileservers don't maintain
> state with each other and thus a lock on a R/O volume file on one
> fileserver will not be be visible to someone looking at the same file on
> another fileserver.
>
> Further, the server may return an error if you try it.
>
> Fix this by doing what other AFS clients do and handle filelocking on R/O
> volume files entirely within the client and don't touch the server.
>
> Fixes: 6c6c1d63c243 ("afs: Provide mount-time configurable byte-range file locking emulation")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>  fs/afs/super.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/afs/super.c b/fs/afs/super.c
> index 95d713074dc8..e95fb4cb4fcd 100644
> --- a/fs/afs/super.c
> +++ b/fs/afs/super.c
> @@ -407,6 +407,8 @@ static int afs_validate_fc(struct fs_context *fc)
>                         return PTR_ERR(volume);
>
>                 ctx->volume = volume;
> +               if (volume->type != AFSVL_RWVOL)
> +                       ctx->flock_mode = afs_flock_mode_local;
>         }
>
>         return 0;

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc