[6.12-rc2 v2 PATCH 0/7] NFS LOCALIO: fixes and various cleanups

[6.12-rc2 v2 PATCH 0/7] NFS LOCALIO: fixes and various cleanups

[Mike Snitzer]

Hi,

The first 3 patches are clear fixes which are needed ASAP (patch 1 is
the same from v1 of these series, patch 2 and 3 are new fixes).

The other 4 patches are cleanups that are more subjective (relative to
them being sent for 6.12-rcX), I'd prefer they go upstream now but I
can carry them until 6.13 if that is how others would like to proceed.

Please note that there are 3 other LOCALIO related fixes that should
be merged into 6.12-rcX:

filemap: Fix bounds checking in filemap_read()
https://lore.kernel.org/all/c6f35a86fe9ae6aa33b2fd3983b4023c2f4f9c13.1726250071.git.trond.myklebust@hammerspace.com/T/
- still needed, Willy or Christian can you please pick this up?

filemap: filemap_read() should check that the offset is positive or zero
- Christian has staged this in linux-next via fs-next

sunrpc: fix prog selection loop in svc_process_common
- Anna has acknowledged the need for this fix but it isn't staged yet

Thanks,
Mike

Mike Snitzer (7):
  nfs_common: fix race in NFS calls to nfsd_file_put_local() and
    nfsd_serv_put()
  nfs_common: fix Kconfig for NFS_COMMON_LOCALIO_SUPPORT
  nfsd/localio: fix nfsd_file tracepoints to handle NULL rqstp
  nfs/localio: remove redundant suid/sgid handling
  nfs/localio: eliminate unnecessary kref in nfs_local_fsync_ctx
  nfs/localio: remove extra indirect nfs_to call to check
    {read,write}_iter
  nfs/localio: eliminate need for nfs_local_fsync_work forward
    declaration

 fs/Kconfig                 |  2 +-
 fs/nfs/localio.c           | 96 ++++++++++++++++----------------------
 fs/nfs_common/nfslocalio.c |  5 +-
 fs/nfsd/filecache.c        |  2 +-
 fs/nfsd/localio.c          |  2 +-
 fs/nfsd/nfssvc.c           |  4 +-
 fs/nfsd/trace.h            |  6 +--
 include/linux/nfslocalio.h | 15 ++++++
 8 files changed, 68 insertions(+), 64 deletions(-)

-- 
2.44.0

[6.12-rc2 v2 PATCH 1/7] nfs_common: fix race in NFS calls to nfsd_file_put_local() and nfsd_serv_put()

[Mike Snitzer]

Add nfs_to_nfsd_file_put_local() interface to fix race with nfsd
module unload.  Similarly, use RCU around nfs_open_local_fh()'s error
path call to nfs_to->nfsd_serv_put().  Holding RCU ensures that NFS
will safely _call and return_ from its nfs_to calls into the NFSD
functions nfsd_file_put_local() and nfsd_serv_put().

Otherwise, if RCU isn't used then there is a narrow window when NFS's
reference for the nfsd_file and nfsd_serv are dropped and the NFSD
module could be unloaded, which could result in a crash from the
return instruction for either nfs_to->nfsd_file_put_local() or
nfs_to->nfsd_serv_put().

Reported-by: NeilBrown <neilb@suse.de>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c           |  6 +++---
 fs/nfs_common/nfslocalio.c |  5 ++++-
 fs/nfsd/filecache.c        |  2 +-
 fs/nfsd/localio.c          |  2 +-
 fs/nfsd/nfssvc.c           |  4 ++--
 include/linux/nfslocalio.h | 15 +++++++++++++++
 6 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index c29cdf51c458..d124c265b8fd 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -341,7 +341,7 @@ nfs_local_pgio_release(struct nfs_local_kiocb *iocb)
 {
 	struct nfs_pgio_header *hdr = iocb->hdr;
 
-	nfs_to->nfsd_file_put_local(iocb->localio);
+	nfs_to_nfsd_file_put_local(iocb->localio);
 	nfs_local_iocb_free(iocb);
 	nfs_local_hdr_release(hdr, hdr->task.tk_ops);
 }
@@ -622,7 +622,7 @@ int nfs_local_doio(struct nfs_client *clp, struct nfsd_file *localio,
 	}
 out:
 	if (status != 0) {
-		nfs_to->nfsd_file_put_local(localio);
+		nfs_to_nfsd_file_put_local(localio);
 		hdr->task.tk_status = status;
 		nfs_local_hdr_release(hdr, call_ops);
 	}
@@ -673,7 +673,7 @@ nfs_local_release_commit_data(struct nfsd_file *localio,
 		struct nfs_commit_data *data,
 		const struct rpc_call_ops *call_ops)
 {
-	nfs_to->nfsd_file_put_local(localio);
+	nfs_to_nfsd_file_put_local(localio);
 	call_ops->rpc_call_done(&data->task, data);
 	call_ops->rpc_release(data);
 }
diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
index 42b479b9191f..5c8ce5066c16 100644
--- a/fs/nfs_common/nfslocalio.c
+++ b/fs/nfs_common/nfslocalio.c
@@ -142,8 +142,11 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *uuid,
 	/* We have an implied reference to net thanks to nfsd_serv_try_get */
 	localio = nfs_to->nfsd_open_local_fh(net, uuid->dom, rpc_clnt,
 					     cred, nfs_fh, fmode);
-	if (IS_ERR(localio))
+	if (IS_ERR(localio)) {
+		rcu_read_lock();
 		nfs_to->nfsd_serv_put(net);
+		rcu_read_unlock();
+	}
 	return localio;
 }
 EXPORT_SYMBOL_GPL(nfs_open_local_fh);
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 19bb88c7eebd..53070e1de3d9 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -398,7 +398,7 @@ nfsd_file_put(struct nfsd_file *nf)
  * reference to the associated nn->nfsd_serv.
  */
 void
-nfsd_file_put_local(struct nfsd_file *nf)
+nfsd_file_put_local(struct nfsd_file *nf) __must_hold(rcu)
 {
 	struct net *net = nf->nf_net;
 
diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
index 291e9c69cae4..f441cb9f74d5 100644
--- a/fs/nfsd/localio.c
+++ b/fs/nfsd/localio.c
@@ -53,7 +53,7 @@ void nfsd_localio_ops_init(void)
  *
  * On successful return, returned nfsd_file will have its nf_net member
  * set. Caller (NFS client) is responsible for calling nfsd_serv_put and
- * nfsd_file_put (via nfs_to->nfsd_file_put_local).
+ * nfsd_file_put (via nfs_to_nfsd_file_put_local).
  */
 struct nfsd_file *
 nfsd_open_local_fh(struct net *net, struct auth_domain *dom,
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index e236135ddc63..47172b407be8 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -214,14 +214,14 @@ int nfsd_minorversion(struct nfsd_net *nn, u32 minorversion, enum vers_op change
 	return 0;
 }
 
-bool nfsd_serv_try_get(struct net *net)
+bool nfsd_serv_try_get(struct net *net) __must_hold(rcu)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	return (nn && percpu_ref_tryget_live(&nn->nfsd_serv_ref));
 }
 
-void nfsd_serv_put(struct net *net)
+void nfsd_serv_put(struct net *net) __must_hold(rcu)
 {
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
index b353abe00357..b0dd9b1eef4f 100644
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -65,10 +65,25 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *,
 		   struct rpc_clnt *, const struct cred *,
 		   const struct nfs_fh *, const fmode_t);
 
+static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
+{
+	/*
+	 * Once reference to nfsd_serv is dropped, NFSD could be
+	 * unloaded, so ensure safe return from nfsd_file_put_local()
+	 * by always taking RCU.
+	 */
+	rcu_read_lock();
+	nfs_to->nfsd_file_put_local(localio);
+	rcu_read_unlock();
+}
+
 #else   /* CONFIG_NFS_LOCALIO */
 static inline void nfsd_localio_ops_init(void)
 {
 }
+static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
+{
+}
 #endif  /* CONFIG_NFS_LOCALIO */
 
 #endif  /* __LINUX_NFSLOCALIO_H */
-- 
2.44.0

[6.12-rc2 v2 PATCH 2/7] nfs_common: fix Kconfig for NFS_COMMON_LOCALIO_SUPPORT

[Mike Snitzer]

The 'default n' that was in NFS_COMMON_LOCALIO_SUPPORT caused these
extra defaults to be missed:
        default y if NFSD=y || NFS_FS=y
	default m if NFSD=m && NFS_FS=m

Remove the 'default n' for NFS_COMMON_LOCALIO_SUPPORT so that the
correct tristate is selected based on how NFSD and NFS_FS are
configured.  This fixes the reported case where NFS_FS=y but
NFS_COMMON_LOCALIO_SUPPORT=m, it is now correctly set to =y.

In addition, add extra 'depends on NFS_LOCALIO' to
NFS_COMMON_LOCALIO_SUPPORT so that if NFS_LOCALIO isn't set then
NFS_COMMON_LOCALIO_SUPPORT will not be either.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202410031944.hMCFY9BO-lkp@intel.com/
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index 24d4e4b419d1..da8ad9aba3e9 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -384,7 +384,7 @@ config NFS_COMMON
 
 config NFS_COMMON_LOCALIO_SUPPORT
 	tristate
-	default n
+	depends on NFS_LOCALIO
 	default y if NFSD=y || NFS_FS=y
 	default m if NFSD=m && NFS_FS=m
 	select SUNRPC
-- 
2.44.0

[6.12-rc2 v2 PATCH 3/7] nfsd/localio: fix nfsd_file tracepoints to handle NULL rqstp

[Mike Snitzer]

Otherwise nfsd_file_acquire, nfsd_file_insert_err, and
nfsd_file_cons_err will hit a NULL pointer when they are enabled and
LOCALIO used.

Example trace output (note xid is 0x0 and LOCALIO flag set):
 nfsd_file_acquire: xid=0x0 inode=0000000069a1b2e7
 may_flags=WRITE|LOCALIO ref=1 nf_flags=HASHED|GC nf_may=WRITE
 nf_file=0000000070123234 status=0

Fixes: c63f0e48febf ("nfsd: add nfsd_file_acquire_local()")
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfsd/trace.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index c625966cfcf3..b8470d4cbe99 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -1113,7 +1113,7 @@ TRACE_EVENT(nfsd_file_acquire,
 	),
 
 	TP_fast_assign(
-		__entry->xid = be32_to_cpu(rqstp->rq_xid);
+		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
 		__entry->inode = inode;
 		__entry->may_flags = may_flags;
 		__entry->nf_ref = nf ? refcount_read(&nf->nf_ref) : 0;
@@ -1147,7 +1147,7 @@ TRACE_EVENT(nfsd_file_insert_err,
 		__field(long, error)
 	),
 	TP_fast_assign(
-		__entry->xid = be32_to_cpu(rqstp->rq_xid);
+		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
 		__entry->inode = inode;
 		__entry->may_flags = may_flags;
 		__entry->error = error;
@@ -1177,7 +1177,7 @@ TRACE_EVENT(nfsd_file_cons_err,
 		__field(const void *, nf_file)
 	),
 	TP_fast_assign(
-		__entry->xid = be32_to_cpu(rqstp->rq_xid);
+		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
 		__entry->inode = inode;
 		__entry->may_flags = may_flags;
 		__entry->nf_ref = refcount_read(&nf->nf_ref);
-- 
2.44.0

[6.12-rc2 v2 PATCH 4/7] nfs/localio: remove redundant suid/sgid handling

[Mike Snitzer]

From: Mike Snitzer <snitzer@hammerspace.com>

nfs_writeback_done() will take care of suid/sgid corner case.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index d124c265b8fd..88b6658b93fc 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -521,12 +521,7 @@ nfs_local_write_done(struct nfs_local_kiocb *iocb, long status)
 	}
 	if (status < 0)
 		nfs_reset_boot_verifier(inode);
-	else if (nfs_should_remove_suid(inode)) {
-		/* Deal with the suid/sgid bit corner case */
-		spin_lock(&inode->i_lock);
-		nfs_set_cache_invalid(inode, NFS_INO_INVALID_MODE);
-		spin_unlock(&inode->i_lock);
-	}
+
 	nfs_local_pgio_done(hdr, status);
 }
 
-- 
2.44.0

[6.12-rc2 v2 PATCH 5/7] nfs/localio: eliminate unnecessary kref in nfs_local_fsync_ctx

[Mike Snitzer]

nfs_local_commit() doesn't need async cleanup of nfs_local_fsync_ctx,
so there is no need to use a kref.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 88b6658b93fc..2f302b03b253 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -43,7 +43,6 @@ struct nfs_local_fsync_ctx {
 	struct nfsd_file	*localio;
 	struct nfs_commit_data	*data;
 	struct work_struct	work;
-	struct kref		kref;
 	struct completion	*done;
 };
 static void nfs_local_fsync_work(struct work_struct *work);
@@ -683,30 +682,17 @@ nfs_local_fsync_ctx_alloc(struct nfs_commit_data *data,
 		ctx->localio = localio;
 		ctx->data = data;
 		INIT_WORK(&ctx->work, nfs_local_fsync_work);
-		kref_init(&ctx->kref);
 		ctx->done = NULL;
 	}
 	return ctx;
 }
 
-static void
-nfs_local_fsync_ctx_kref_free(struct kref *kref)
-{
-	kfree(container_of(kref, struct nfs_local_fsync_ctx, kref));
-}
-
-static void
-nfs_local_fsync_ctx_put(struct nfs_local_fsync_ctx *ctx)
-{
-	kref_put(&ctx->kref, nfs_local_fsync_ctx_kref_free);
-}
-
 static void
 nfs_local_fsync_ctx_free(struct nfs_local_fsync_ctx *ctx)
 {
 	nfs_local_release_commit_data(ctx->localio, ctx->data,
 				      ctx->data->task.tk_ops);
-	nfs_local_fsync_ctx_put(ctx);
+	kfree(ctx);
 }
 
 static void
@@ -739,7 +725,7 @@ int nfs_local_commit(struct nfsd_file *localio,
 	}
 
 	nfs_local_init_commit(data, call_ops);
-	kref_get(&ctx->kref);
+
 	if (how & FLUSH_SYNC) {
 		DECLARE_COMPLETION_ONSTACK(done);
 		ctx->done = &done;
@@ -747,6 +733,6 @@ int nfs_local_commit(struct nfsd_file *localio,
 		wait_for_completion(&done);
 	} else
 		queue_work(nfsiod_workqueue, &ctx->work);
-	nfs_local_fsync_ctx_put(ctx);
+
 	return 0;
 }
-- 
2.44.0

[6.12-rc2 v2 PATCH 6/7] nfs/localio: remove extra indirect nfs_to call to check {read,write}_iter

[Mike Snitzer]

Push the read_iter and write_iter availability checks down to
nfs_do_local_read and nfs_do_local_write respectively.

This eliminates a redundant nfs_to->nfsd_file_file() call.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 2f302b03b253..12d3e200156c 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -273,7 +273,7 @@ nfs_local_iocb_free(struct nfs_local_kiocb *iocb)
 
 static struct nfs_local_kiocb *
 nfs_local_iocb_alloc(struct nfs_pgio_header *hdr,
-		     struct nfsd_file *localio, gfp_t flags)
+		     struct file *file, gfp_t flags)
 {
 	struct nfs_local_kiocb *iocb;
 
@@ -286,9 +286,8 @@ nfs_local_iocb_alloc(struct nfs_pgio_header *hdr,
 		kfree(iocb);
 		return NULL;
 	}
-	init_sync_kiocb(&iocb->kiocb, nfs_to->nfsd_file_file(localio));
+	init_sync_kiocb(&iocb->kiocb, file);
 	iocb->kiocb.ki_pos = hdr->args.offset;
-	iocb->localio = localio;
 	iocb->hdr = hdr;
 	iocb->kiocb.ki_flags &= ~IOCB_APPEND;
 	return iocb;
@@ -389,13 +388,19 @@ nfs_do_local_read(struct nfs_pgio_header *hdr,
 		  const struct rpc_call_ops *call_ops)
 {
 	struct nfs_local_kiocb *iocb;
+	struct file *file = nfs_to->nfsd_file_file(localio);
+
+	/* Don't support filesystems without read_iter */
+	if (!file->f_op->read_iter)
+		return -EAGAIN;
 
 	dprintk("%s: vfs_read count=%u pos=%llu\n",
 		__func__, hdr->args.count, hdr->args.offset);
 
-	iocb = nfs_local_iocb_alloc(hdr, localio, GFP_KERNEL);
+	iocb = nfs_local_iocb_alloc(hdr, file, GFP_KERNEL);
 	if (iocb == NULL)
 		return -ENOMEM;
+	iocb->localio = localio;
 
 	nfs_local_pgio_init(hdr, call_ops);
 	hdr->res.eof = false;
@@ -558,14 +563,20 @@ nfs_do_local_write(struct nfs_pgio_header *hdr,
 		   const struct rpc_call_ops *call_ops)
 {
 	struct nfs_local_kiocb *iocb;
+	struct file *file = nfs_to->nfsd_file_file(localio);
+
+	/* Don't support filesystems without write_iter */
+	if (!file->f_op->write_iter)
+		return -EAGAIN;
 
 	dprintk("%s: vfs_write count=%u pos=%llu %s\n",
 		__func__, hdr->args.count, hdr->args.offset,
 		(hdr->args.stable == NFS_UNSTABLE) ?  "unstable" : "stable");
 
-	iocb = nfs_local_iocb_alloc(hdr, localio, GFP_NOIO);
+	iocb = nfs_local_iocb_alloc(hdr, file, GFP_NOIO);
 	if (iocb == NULL)
 		return -ENOMEM;
+	iocb->localio = localio;
 
 	switch (hdr->args.stable) {
 	default:
@@ -591,16 +602,9 @@ int nfs_local_doio(struct nfs_client *clp, struct nfsd_file *localio,
 		   const struct rpc_call_ops *call_ops)
 {
 	int status = 0;
-	struct file *filp = nfs_to->nfsd_file_file(localio);
 
 	if (!hdr->args.count)
 		return 0;
-	/* Don't support filesystems without read_iter/write_iter */
-	if (!filp->f_op->read_iter || !filp->f_op->write_iter) {
-		nfs_local_disable(clp);
-		status = -EAGAIN;
-		goto out;
-	}
 
 	switch (hdr->rw_mode) {
 	case FMODE_READ:
@@ -614,8 +618,10 @@ int nfs_local_doio(struct nfs_client *clp, struct nfsd_file *localio,
 			hdr->rw_mode);
 		status = -EINVAL;
 	}
-out:
+
 	if (status != 0) {
+		if (status == -EAGAIN)
+			nfs_local_disable(clp);
 		nfs_to_nfsd_file_put_local(localio);
 		hdr->task.tk_status = status;
 		nfs_local_hdr_release(hdr, call_ops);
-- 
2.44.0

[6.12-rc2 v2 PATCH 7/7] nfs/localio: eliminate need for nfs_local_fsync_work forward declaration

[Mike Snitzer]

Move nfs_local_fsync_ctx_alloc() after nfs_local_fsync_work().

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 12d3e200156c..98cd572426b4 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -45,7 +45,6 @@ struct nfs_local_fsync_ctx {
 	struct work_struct	work;
 	struct completion	*done;
 };
-static void nfs_local_fsync_work(struct work_struct *work);
 
 static bool localio_enabled __read_mostly = true;
 module_param(localio_enabled, bool, 0644);
@@ -678,21 +677,6 @@ nfs_local_release_commit_data(struct nfsd_file *localio,
 	call_ops->rpc_release(data);
 }
 
-static struct nfs_local_fsync_ctx *
-nfs_local_fsync_ctx_alloc(struct nfs_commit_data *data,
-			  struct nfsd_file *localio, gfp_t flags)
-{
-	struct nfs_local_fsync_ctx *ctx = kmalloc(sizeof(*ctx), flags);
-
-	if (ctx != NULL) {
-		ctx->localio = localio;
-		ctx->data = data;
-		INIT_WORK(&ctx->work, nfs_local_fsync_work);
-		ctx->done = NULL;
-	}
-	return ctx;
-}
-
 static void
 nfs_local_fsync_ctx_free(struct nfs_local_fsync_ctx *ctx)
 {
@@ -717,6 +701,21 @@ nfs_local_fsync_work(struct work_struct *work)
 	nfs_local_fsync_ctx_free(ctx);
 }
 
+static struct nfs_local_fsync_ctx *
+nfs_local_fsync_ctx_alloc(struct nfs_commit_data *data,
+			  struct nfsd_file *localio, gfp_t flags)
+{
+	struct nfs_local_fsync_ctx *ctx = kmalloc(sizeof(*ctx), flags);
+
+	if (ctx != NULL) {
+		ctx->localio = localio;
+		ctx->data = data;
+		INIT_WORK(&ctx->work, nfs_local_fsync_work);
+		ctx->done = NULL;
+	}
+	return ctx;
+}
+
 int nfs_local_commit(struct nfsd_file *localio,
 		     struct nfs_commit_data *data,
 		     const struct rpc_call_ops *call_ops, int how)
-- 
2.44.0

Re: [6.12-rc2 v2 PATCH 0/7] NFS LOCALIO: fixes and various cleanups

[Anna Schumaker]

Hi Mike,

On 10/3/24 3:34 PM, Mike Snitzer wrote:
> Hi,
> 
> The first 3 patches are clear fixes which are needed ASAP (patch 1 is
> the same from v1 of these series, patch 2 and 3 are new fixes).
> 
> The other 4 patches are cleanups that are more subjective (relative to
> them being sent for 6.12-rcX), I'd prefer they go upstream now but I
> can carry them until 6.13 if that is how others would like to proceed.

Thanks for the patches! I'm planning to take the 3 bugfixes for now, and I
think we should save the cleanups for 6.13.

Thanks,
Anna

> 
> Please note that there are 3 other LOCALIO related fixes that should
> be merged into 6.12-rcX:
> 
> filemap: Fix bounds checking in filemap_read()
> https://lore.kernel.org/all/c6f35a86fe9ae6aa33b2fd3983b4023c2f4f9c13.1726250071.git.trond.myklebust@hammerspace.com/T/
> - still needed, Willy or Christian can you please pick this up?
> 
> filemap: filemap_read() should check that the offset is positive or zero
> - Christian has staged this in linux-next via fs-next
> 
> sunrpc: fix prog selection loop in svc_process_common
> - Anna has acknowledged the need for this fix but it isn't staged yet
> 
> Thanks,
> Mike
> 
> Mike Snitzer (7):
>   nfs_common: fix race in NFS calls to nfsd_file_put_local() and
>     nfsd_serv_put()
>   nfs_common: fix Kconfig for NFS_COMMON_LOCALIO_SUPPORT
>   nfsd/localio: fix nfsd_file tracepoints to handle NULL rqstp
>   nfs/localio: remove redundant suid/sgid handling
>   nfs/localio: eliminate unnecessary kref in nfs_local_fsync_ctx
>   nfs/localio: remove extra indirect nfs_to call to check
>     {read,write}_iter
>   nfs/localio: eliminate need for nfs_local_fsync_work forward
>     declaration
> 
>  fs/Kconfig                 |  2 +-
>  fs/nfs/localio.c           | 96 ++++++++++++++++----------------------
>  fs/nfs_common/nfslocalio.c |  5 +-
>  fs/nfsd/filecache.c        |  2 +-
>  fs/nfsd/localio.c          |  2 +-
>  fs/nfsd/nfssvc.c           |  4 +-
>  fs/nfsd/trace.h            |  6 +--
>  include/linux/nfslocalio.h | 15 ++++++
>  8 files changed, 68 insertions(+), 64 deletions(-)
> 

Re: [6.12-rc2 v2 PATCH 3/7] nfsd/localio: fix nfsd_file tracepoints to handle NULL rqstp

[Chuck Lever]

On Thu, Oct 03, 2024 at 03:35:00PM -0400, Mike Snitzer wrote:
> Otherwise nfsd_file_acquire, nfsd_file_insert_err, and
> nfsd_file_cons_err will hit a NULL pointer when they are enabled and
> LOCALIO used.
> 
> Example trace output (note xid is 0x0 and LOCALIO flag set):
>  nfsd_file_acquire: xid=0x0 inode=0000000069a1b2e7
>  may_flags=WRITE|LOCALIO ref=1 nf_flags=HASHED|GC nf_may=WRITE
>  nf_file=0000000070123234 status=0
> 
> Fixes: c63f0e48febf ("nfsd: add nfsd_file_acquire_local()")
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfsd/trace.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index c625966cfcf3..b8470d4cbe99 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1113,7 +1113,7 @@ TRACE_EVENT(nfsd_file_acquire,
>  	),
>  
>  	TP_fast_assign(
> -		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
>  		__entry->inode = inode;
>  		__entry->may_flags = may_flags;
>  		__entry->nf_ref = nf ? refcount_read(&nf->nf_ref) : 0;

A future auditor of this code might wonder why trace_nfsd_fh_verify()
takes this approach instead:

 197 TRACE_EVENT_CONDITION(nfsd_fh_verify,
 198         TP_PROTO(
 199                 const struct svc_rqst *rqstp,
 200                 const struct svc_fh *fhp,
 201                 umode_t type,
 202                 int access
 203         ),
 204         TP_ARGS(rqstp, fhp, type, access),
 205         TP_CONDITION(rqstp != NULL),

The answer is that trace_nfsd_fh_verify() uses @rqstp in its
TP_STRUCT__entry () clause. Thus the entire nfsd_fh_verify() trace
point has to be short-circuited if @rqstp is NULL.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


> @@ -1147,7 +1147,7 @@ TRACE_EVENT(nfsd_file_insert_err,
>  		__field(long, error)
>  	),
>  	TP_fast_assign(
> -		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
>  		__entry->inode = inode;
>  		__entry->may_flags = may_flags;
>  		__entry->error = error;
> @@ -1177,7 +1177,7 @@ TRACE_EVENT(nfsd_file_cons_err,
>  		__field(const void *, nf_file)
>  	),
>  	TP_fast_assign(
> -		__entry->xid = be32_to_cpu(rqstp->rq_xid);
> +		__entry->xid = rqstp ? be32_to_cpu(rqstp->rq_xid) : 0;
>  		__entry->inode = inode;
>  		__entry->may_flags = may_flags;
>  		__entry->nf_ref = refcount_read(&nf->nf_ref);
> -- 
> 2.44.0
> 

-- 
Chuck Lever