| Message ID | 20240701025802.22985-7-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: provide simpler interface for LOCALIO access | expand |
On Mon, 2024-07-01 at 12:53 +1000, NeilBrown wrote: > nfsd_file_acquire_local() can be used to look up a file by filehandle > without having a struct rqst. This can be used by NFS LOCALIO to allow > the NFS client to by the NFS protocol to directly access a file provided > by the NFS server which is running in the same kernel. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/filecache.c | 54 ++++++++++++++++++++++++++++++++++++++++----- > fs/nfsd/filecache.h | 4 ++++ > fs/nfsd/nfsfh.c | 2 +- > fs/nfsd/nfsfh.h | 5 +++++ > 4 files changed, 59 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index ad9083ca144b..87f965d2574b 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -977,7 +977,10 @@ nfsd_file_is_cached(struct inode *inode) > } > > static __be32 > -nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > +nfsd_file_do_acquire(struct svc_rqst *rqstp, struct nfsd_net *nn, > + struct svc_cred *cred, int nfs_vers, > + struct auth_domain *client, > + struct svc_fh *fhp, > unsigned int may_flags, struct file *file, > struct nfsd_file **pnf, bool want_gc) > { > @@ -991,7 +994,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > int ret; > > retry: > - status = fh_verify(rqstp, fhp, S_IFREG, > + status = __fh_verify(rqstp, nn, cred, nfs_vers, client, fhp, S_IFREG, > may_flags|NFSD_MAY_OWNER_OVERRIDE); > if (status != nfs_ok) > return status; > @@ -1139,7 +1142,8 @@ __be32 > nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned int may_flags, struct nfsd_file **pnf) > { > - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, true); > + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, > + fhp, may_flags, NULL, pnf, true); > } > > /** > @@ -1163,7 +1167,46 @@ __be32 > nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned int may_flags, struct nfsd_file **pnf) > { > - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, false); > + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, fhp, > + may_flags, NULL, pnf, false); > +} > + > +/** > + * nfsd_file_acquire_local - Get a struct nfsd_file with an open file for localio > + * @nn: The nfsd network namespace in which to perform a lookup > + * @cred: the user credential with which to validate access > + * @nfs_vers: NFS version number to assume for request > + * @client: the auth_domain for LOCALIO lookup > + * @fhp: the NFS filehandle of the file to be opened > + * @may_flags: NFSD_MAY_ settings for the file > + * @pnf: OUT: new or found "struct nfsd_file" object > + * > + * This file lookup interface provide access to a file given the > + * filehandle and credential. No connection-based authorisation > + * is performed and in that way it is quite different to other > + * file access mediated by nfsd. It allows a kernel module such as the NFS > + * client to reach across network and filesystem namespaces to access > + * a file. The security implications of this should be carefully > + * considered before use. > + * > + * The nfsd_file_object returned by this API is reference-counted > + * but not garbage-collected. The object is unhashed after the > + * final nfsd_file_put(). > + * > + * Return values: > + * %nfs_ok - @pnf points to an nfsd_file with its reference > + * count boosted. > + * > + * On error, an nfsstat value in network byte order is returned. > + */ > +__be32 > +nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred, > + int nfs_vers, struct auth_domain *client, > + struct svc_fh *fhp, > + unsigned int may_flags, struct nfsd_file **pnf) > +{ > + return nfsd_file_do_acquire(NULL, nn, cred, nfs_vers, client, > + fhp, may_flags, NULL, pnf, false); It still seems to me like these should be garbage-collected. Neil, in an earlier email you mentioned that the client could hold onto the nfsd_file reference over several operations and then put it. That would be fine too, but I'm unclear on how the client will manage this. Does the client have a way to keep track of the nfsd_file over several operations to the same inode? Even then, I still think we're probably better off just garbage collecting thse, since it seems likely that they will end up being reused in many cases. > } > > /** > @@ -1189,7 +1232,8 @@ nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned int may_flags, struct file *file, > struct nfsd_file **pnf) > { > - return nfsd_file_do_acquire(rqstp, fhp, may_flags, file, pnf, false); > + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, > + fhp, may_flags, file, pnf, false); > } > > /* > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h > index c61884def906..d179dbae98e3 100644 > --- a/fs/nfsd/filecache.h > +++ b/fs/nfsd/filecache.h > @@ -65,5 +65,9 @@ __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > __be32 nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp, > unsigned int may_flags, struct file *file, > struct nfsd_file **nfp); > +__be32 nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred, > + int nfs_vers, struct auth_domain *client, > + struct svc_fh *fhp, > + unsigned int may_flags, struct nfsd_file **pnf); > int nfsd_file_cache_stats_show(struct seq_file *m, void *v); > #endif /* _FS_NFSD_FILECACHE_H */ > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index fb5a23060a4c..fa7e358d91ab 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -328,7 +328,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn, > * @access is formed from the NFSD_MAY_* constants defined in > * fs/nfsd/vfs.h. > */ > -static __be32 > +__be32 > __fh_verify(struct svc_rqst *rqstp, > struct nfsd_net *nn, struct svc_cred *cred, > int nfs_vers, struct auth_domain *client, > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > index 6ebdf7ea27bf..a2d9962f1bf8 100644 > --- a/fs/nfsd/nfsfh.h > +++ b/fs/nfsd/nfsfh.h > @@ -214,7 +214,12 @@ extern char * SVCFH_fmt(struct svc_fh *fhp); > /* > * Function prototypes > */ > +struct nfsd_net; > __be32 fh_verify(struct svc_rqst *, struct svc_fh *, umode_t, int); > +__be32 __fh_verify(struct svc_rqst *rqstp, > + struct nfsd_net *nn, struct svc_cred *cred, > + int nfs_vers, struct auth_domain *client, > + struct svc_fh *fhp, umode_t type, int access); > __be32 fh_compose(struct svc_fh *, struct svc_export *, struct dentry *, struct svc_fh *); > __be32 fh_update(struct svc_fh *); > void fh_put(struct svc_fh *);
On Mon, 01 Jul 2024, Jeff Layton wrote: > > Neil, in an earlier email you mentioned that the client could hold onto > the nfsd_file reference over several operations and then put it. That > would be fine too, but I'm unclear on how the client will manage this. > Does the client have a way to keep track of the nfsd_file over several > operations to the same inode? Looking at [PATCH v10 13/19] nfs: add "localio" support you can see that struct file *local_filp; is added to "struct nfs_open_context". An nfs_open_context is stored in file->private_data and is attached to the inode via nfsi->open_files. It holds the nfs state for any file open on the NFS filesystem.. ->local_filp is set the first time nfs_local_file_open_cache() is called and remains set until the final __put_nfs_open_context() call destroys the context. So it lasts as long as the NFS file is open. Note that only one successful ->nfsd_open_local_fh() call is made for each opened NFS file. All IO then uses the "struct file*" with no further reference to nfsd. If we stored an nfsd_file in the nfs_open_context, either as well as the 'struct file*' or instead of, then we could call nfsd_file_put() when the nfs file is closed. That seems to be the correct lifetime and matches (almost) exactly what happens with NFSv4 where OPEN and CLOSE are send over the wire. > > Even then, I still think we're probably better off just garbage > collecting thse, since it seems likely that they will end up being > reused in many cases. Why does this logic apply to localio, but not to normal NFSv4 access? NeilBrown
On Tue, 2024-07-02 at 09:55 +1000, NeilBrown wrote: > On Mon, 01 Jul 2024, Jeff Layton wrote: > > > > Neil, in an earlier email you mentioned that the client could hold onto > > the nfsd_file reference over several operations and then put it. That > > would be fine too, but I'm unclear on how the client will manage this. > > Does the client have a way to keep track of the nfsd_file over several > > operations to the same inode? > > Looking at > [PATCH v10 13/19] nfs: add "localio" support > > you can see that > struct file *local_filp; > is added to "struct nfs_open_context". An nfs_open_context is stored > in file->private_data and is attached to the inode via nfsi->open_files. > It holds the nfs state for any file open on the NFS filesystem.. > > ->local_filp is set the first time nfs_local_file_open_cache() is called > and remains set until the final __put_nfs_open_context() call destroys > the context. So it lasts as long as the NFS file is open. Note that > only one successful ->nfsd_open_local_fh() call is made for each opened > NFS file. All IO then uses the "struct file*" with no further reference > to nfsd. > > If we stored an nfsd_file in the nfs_open_context, either as well as > the 'struct file*' or instead of, then we could call nfsd_file_put() > when the nfs file is closed. That seems to be the correct lifetime and > matches (almost) exactly what happens with NFSv4 where OPEN and CLOSE > are send over the wire. > Ok. > > > > Even then, I still think we're probably better off just garbage > > collecting thse, since it seems likely that they will end up being > > reused in many cases. > > Why does this logic apply to localio, but not to normal NFSv4 access? > The main idea with the filecache was to keep open files around for a little while to reduce the open/close overhead between v3 RPCs. The argument for not caching files with v4 is that we have an open stateid that pins the open file in place, so we don't need to do that there. Hanging an nfsd_file off of the nfs client's open_context sounds like a reasonable alternative.
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index ad9083ca144b..87f965d2574b 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -977,7 +977,10 @@ nfsd_file_is_cached(struct inode *inode) } static __be32 -nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, +nfsd_file_do_acquire(struct svc_rqst *rqstp, struct nfsd_net *nn, + struct svc_cred *cred, int nfs_vers, + struct auth_domain *client, + struct svc_fh *fhp, unsigned int may_flags, struct file *file, struct nfsd_file **pnf, bool want_gc) { @@ -991,7 +994,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, int ret; retry: - status = fh_verify(rqstp, fhp, S_IFREG, + status = __fh_verify(rqstp, nn, cred, nfs_vers, client, fhp, S_IFREG, may_flags|NFSD_MAY_OWNER_OVERRIDE); if (status != nfs_ok) return status; @@ -1139,7 +1142,8 @@ __be32 nfsd_file_acquire_gc(struct svc_rqst *rqstp, struct svc_fh *fhp, unsigned int may_flags, struct nfsd_file **pnf) { - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, true); + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, + fhp, may_flags, NULL, pnf, true); } /** @@ -1163,7 +1167,46 @@ __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, unsigned int may_flags, struct nfsd_file **pnf) { - return nfsd_file_do_acquire(rqstp, fhp, may_flags, NULL, pnf, false); + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, fhp, + may_flags, NULL, pnf, false); +} + +/** + * nfsd_file_acquire_local - Get a struct nfsd_file with an open file for localio + * @nn: The nfsd network namespace in which to perform a lookup + * @cred: the user credential with which to validate access + * @nfs_vers: NFS version number to assume for request + * @client: the auth_domain for LOCALIO lookup + * @fhp: the NFS filehandle of the file to be opened + * @may_flags: NFSD_MAY_ settings for the file + * @pnf: OUT: new or found "struct nfsd_file" object + * + * This file lookup interface provide access to a file given the + * filehandle and credential. No connection-based authorisation + * is performed and in that way it is quite different to other + * file access mediated by nfsd. It allows a kernel module such as the NFS + * client to reach across network and filesystem namespaces to access + * a file. The security implications of this should be carefully + * considered before use. + * + * The nfsd_file_object returned by this API is reference-counted + * but not garbage-collected. The object is unhashed after the + * final nfsd_file_put(). + * + * Return values: + * %nfs_ok - @pnf points to an nfsd_file with its reference + * count boosted. + * + * On error, an nfsstat value in network byte order is returned. + */ +__be32 +nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred, + int nfs_vers, struct auth_domain *client, + struct svc_fh *fhp, + unsigned int may_flags, struct nfsd_file **pnf) +{ + return nfsd_file_do_acquire(NULL, nn, cred, nfs_vers, client, + fhp, may_flags, NULL, pnf, false); } /** @@ -1189,7 +1232,8 @@ nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp, unsigned int may_flags, struct file *file, struct nfsd_file **pnf) { - return nfsd_file_do_acquire(rqstp, fhp, may_flags, file, pnf, false); + return nfsd_file_do_acquire(rqstp, NULL, NULL, 0, NULL, + fhp, may_flags, file, pnf, false); } /* diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h index c61884def906..d179dbae98e3 100644 --- a/fs/nfsd/filecache.h +++ b/fs/nfsd/filecache.h @@ -65,5 +65,9 @@ __be32 nfsd_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, __be32 nfsd_file_acquire_opened(struct svc_rqst *rqstp, struct svc_fh *fhp, unsigned int may_flags, struct file *file, struct nfsd_file **nfp); +__be32 nfsd_file_acquire_local(struct nfsd_net *nn, struct svc_cred *cred, + int nfs_vers, struct auth_domain *client, + struct svc_fh *fhp, + unsigned int may_flags, struct nfsd_file **pnf); int nfsd_file_cache_stats_show(struct seq_file *m, void *v); #endif /* _FS_NFSD_FILECACHE_H */ diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index fb5a23060a4c..fa7e358d91ab 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -328,7 +328,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct nfsd_net *nn, * @access is formed from the NFSD_MAY_* constants defined in * fs/nfsd/vfs.h. */ -static __be32 +__be32 __fh_verify(struct svc_rqst *rqstp, struct nfsd_net *nn, struct svc_cred *cred, int nfs_vers, struct auth_domain *client, diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h index 6ebdf7ea27bf..a2d9962f1bf8 100644 --- a/fs/nfsd/nfsfh.h +++ b/fs/nfsd/nfsfh.h @@ -214,7 +214,12 @@ extern char * SVCFH_fmt(struct svc_fh *fhp); /* * Function prototypes */ +struct nfsd_net; __be32 fh_verify(struct svc_rqst *, struct svc_fh *, umode_t, int); +__be32 __fh_verify(struct svc_rqst *rqstp, + struct nfsd_net *nn, struct svc_cred *cred, + int nfs_vers, struct auth_domain *client, + struct svc_fh *fhp, umode_t type, int access); __be32 fh_compose(struct svc_fh *, struct svc_export *, struct dentry *, struct svc_fh *); __be32 fh_update(struct svc_fh *); void fh_put(struct svc_fh *);
nfsd_file_acquire_local() can be used to look up a file by filehandle without having a struct rqst. This can be used by NFS LOCALIO to allow the NFS client to by the NFS protocol to directly access a file provided by the NFS server which is running in the same kernel. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/filecache.c | 54 ++++++++++++++++++++++++++++++++++++++++----- fs/nfsd/filecache.h | 4 ++++ fs/nfsd/nfsfh.c | 2 +- fs/nfsd/nfsfh.h | 5 +++++ 4 files changed, 59 insertions(+), 6 deletions(-)