| Message ID | 20250224212051.1756517-1-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/21] procfs: kill ->proc_dops | expand |
On Mon 24-02-25 21:20:31, Al Viro wrote: > It has two possible values - one for "forced lookup" entries, another > for the normal ones. We'd be better off with that as an explicit > flag anyway and in addition to that it opens some fun possibilities > with ->d_op and ->d_flags handling. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> FWIW I went through the patches and I like them. They look mostly straightforward enough to me and as good simplifications. Honza > --- > fs/proc/generic.c | 8 +++++--- > fs/proc/internal.h | 5 +++-- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/proc/generic.c b/fs/proc/generic.c > index 8ec90826a49e..499c2bf67488 100644 > --- a/fs/proc/generic.c > +++ b/fs/proc/generic.c > @@ -254,7 +254,10 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry, > inode = proc_get_inode(dir->i_sb, de); > if (!inode) > return ERR_PTR(-ENOMEM); > - d_set_d_op(dentry, de->proc_dops); > + if (de->flags & PROC_ENTRY_FORCE_LOOKUP) > + d_set_d_op(dentry, &proc_net_dentry_ops); > + else > + d_set_d_op(dentry, &proc_misc_dentry_ops); > return d_splice_alias(inode, dentry); > } > read_unlock(&proc_subdir_lock); > @@ -448,9 +451,8 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, > INIT_LIST_HEAD(&ent->pde_openers); > proc_set_user(ent, (*parent)->uid, (*parent)->gid); > > - ent->proc_dops = &proc_misc_dentry_ops; > /* Revalidate everything under /proc/${pid}/net */ > - if ((*parent)->proc_dops == &proc_net_dentry_ops) > + if ((*parent)->flags & PROC_ENTRY_FORCE_LOOKUP) > pde_force_lookup(ent); > > out: > diff --git a/fs/proc/internal.h b/fs/proc/internal.h > index 1695509370b8..07f75c959173 100644 > --- a/fs/proc/internal.h > +++ b/fs/proc/internal.h > @@ -44,7 +44,6 @@ struct proc_dir_entry { > const struct proc_ops *proc_ops; > const struct file_operations *proc_dir_ops; > }; > - const struct dentry_operations *proc_dops; > union { > const struct seq_operations *seq_ops; > int (*single_show)(struct seq_file *, void *); > @@ -67,6 +66,8 @@ struct proc_dir_entry { > char inline_name[]; > } __randomize_layout; > > +#define PROC_ENTRY_FORCE_LOOKUP 2 /* same space as PROC_ENTRY_PERMANENT */ > + > #define SIZEOF_PDE ( \ > sizeof(struct proc_dir_entry) < 128 ? 128 : \ > sizeof(struct proc_dir_entry) < 192 ? 192 : \ > @@ -346,7 +347,7 @@ extern const struct dentry_operations proc_net_dentry_ops; > static inline void pde_force_lookup(struct proc_dir_entry *pde) > { > /* /proc/net/ entries can be changed under us by setns(CLONE_NEWNET) */ > - pde->proc_dops = &proc_net_dentry_ops; > + pde->flags |= PROC_ENTRY_FORCE_LOOKUP; > } > > /* > -- > 2.39.5 >
On Tue, 25 Feb 2025, Al Viro wrote: > It has two possible values - one for "forced lookup" entries, another > for the normal ones. We'd be better off with that as an explicit > flag anyway and in addition to that it opens some fun possibilities > with ->d_op and ->d_flags handling. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/proc/generic.c | 8 +++++--- > fs/proc/internal.h | 5 +++-- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/proc/generic.c b/fs/proc/generic.c > index 8ec90826a49e..499c2bf67488 100644 > --- a/fs/proc/generic.c > +++ b/fs/proc/generic.c > @@ -254,7 +254,10 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry, > inode = proc_get_inode(dir->i_sb, de); > if (!inode) > return ERR_PTR(-ENOMEM); > - d_set_d_op(dentry, de->proc_dops); > + if (de->flags & PROC_ENTRY_FORCE_LOOKUP) > + d_set_d_op(dentry, &proc_net_dentry_ops); > + else > + d_set_d_op(dentry, &proc_misc_dentry_ops); > return d_splice_alias(inode, dentry); > } > read_unlock(&proc_subdir_lock); > @@ -448,9 +451,8 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, > INIT_LIST_HEAD(&ent->pde_openers); > proc_set_user(ent, (*parent)->uid, (*parent)->gid); > > - ent->proc_dops = &proc_misc_dentry_ops; > /* Revalidate everything under /proc/${pid}/net */ > - if ((*parent)->proc_dops == &proc_net_dentry_ops) > + if ((*parent)->flags & PROC_ENTRY_FORCE_LOOKUP) > pde_force_lookup(ent); > > out: > diff --git a/fs/proc/internal.h b/fs/proc/internal.h > index 1695509370b8..07f75c959173 100644 > --- a/fs/proc/internal.h > +++ b/fs/proc/internal.h > @@ -44,7 +44,6 @@ struct proc_dir_entry { > const struct proc_ops *proc_ops; > const struct file_operations *proc_dir_ops; > }; > - const struct dentry_operations *proc_dops; > union { > const struct seq_operations *seq_ops; > int (*single_show)(struct seq_file *, void *); > @@ -67,6 +66,8 @@ struct proc_dir_entry { > char inline_name[]; > } __randomize_layout; > > +#define PROC_ENTRY_FORCE_LOOKUP 2 /* same space as PROC_ENTRY_PERMANENT */ Should there be a note in include/linux/proc_fs.h say that '2' is in use? Otherwise it seems sensible. Thanks, NeilBrown > + > #define SIZEOF_PDE ( \ > sizeof(struct proc_dir_entry) < 128 ? 128 : \ > sizeof(struct proc_dir_entry) < 192 ? 192 : \ > @@ -346,7 +347,7 @@ extern const struct dentry_operations proc_net_dentry_ops; > static inline void pde_force_lookup(struct proc_dir_entry *pde) > { > /* /proc/net/ entries can be changed under us by setns(CLONE_NEWNET) */ > - pde->proc_dops = &proc_net_dentry_ops; > + pde->flags |= PROC_ENTRY_FORCE_LOOKUP; > } > > /* > -- > 2.39.5 > >
On Wed, 26 Feb 2025, Jan Kara wrote: > On Mon 24-02-25 21:20:31, Al Viro wrote: > > It has two possible values - one for "forced lookup" entries, another > > for the normal ones. We'd be better off with that as an explicit > > flag anyway and in addition to that it opens some fun possibilities > > with ->d_op and ->d_flags handling. > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > FWIW I went through the patches and I like them. They look mostly > straightforward enough to me and as good simplifications. > Ditto. Nice clean-up It might be good to document s_d_flags and particularly the value of setting DCACHE_DONTCACHE. That flag is documented in the list of DCACHE_ flags, but making the connection that it can usefully be put in s_d_flags might be a step to far for many. Thanks, NeilBrown
On Wed, Feb 26, 2025 at 10:30:21AM +1100, NeilBrown wrote: > On Wed, 26 Feb 2025, Jan Kara wrote: > > On Mon 24-02-25 21:20:31, Al Viro wrote: > > > It has two possible values - one for "forced lookup" entries, another > > > for the normal ones. We'd be better off with that as an explicit > > > flag anyway and in addition to that it opens some fun possibilities > > > with ->d_op and ->d_flags handling. > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > > FWIW I went through the patches and I like them. They look mostly > > straightforward enough to me and as good simplifications. > > > > Ditto. Nice clean-up > It might be good to document s_d_flags and particularly the value of > setting DCACHE_DONTCACHE. That flag is documented in the list of > DCACHE_ flags, but making the connection that it can usefully be put in > s_d_flags might be a step to far for many. Probably... The thing is, I'm resurrecting the DCACHE_PERSISTENT patchset (aka tree-in-dcache stuff) and that will have non-trivial interplay there. But you are right - it's probably worth documenting that thing in this series. Re documentation - I'll be posting bits and pieces of dcache docs/audit/proofs of correctness over the next few weeks; at some point we'll need to collect that into a single text, but at the moment what I've got is too disjoint for that. OTOH, we could start a WIP variant in D/f/<something> and have it augmented as we go... Not sure.
On Wed, Feb 26, 2025 at 09:51:32AM +1100, NeilBrown wrote: > > char inline_name[]; > > } __randomize_layout; > > > > +#define PROC_ENTRY_FORCE_LOOKUP 2 /* same space as PROC_ENTRY_PERMANENT */ > > Should there be a note in include/linux/proc_fs.h say that '2' is in > use? Umm... Point, I guess - or we could just put "all other bits are belong to us" there ;-)
On Mon, Feb 24, 2025 at 09:20:31PM +0000, Al Viro wrote: > It has two possible values - one for "forced lookup" entries, another > for the normal ones. We'd be better off with that as an explicit > flag anyway and in addition to that it opens some fun possibilities > with ->d_op and ->d_flags handling. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Reviewed-by: Christian Brauner <brauner@kernel.org>
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 8ec90826a49e..499c2bf67488 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -254,7 +254,10 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry, inode = proc_get_inode(dir->i_sb, de); if (!inode) return ERR_PTR(-ENOMEM); - d_set_d_op(dentry, de->proc_dops); + if (de->flags & PROC_ENTRY_FORCE_LOOKUP) + d_set_d_op(dentry, &proc_net_dentry_ops); + else + d_set_d_op(dentry, &proc_misc_dentry_ops); return d_splice_alias(inode, dentry); } read_unlock(&proc_subdir_lock); @@ -448,9 +451,8 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, INIT_LIST_HEAD(&ent->pde_openers); proc_set_user(ent, (*parent)->uid, (*parent)->gid); - ent->proc_dops = &proc_misc_dentry_ops; /* Revalidate everything under /proc/${pid}/net */ - if ((*parent)->proc_dops == &proc_net_dentry_ops) + if ((*parent)->flags & PROC_ENTRY_FORCE_LOOKUP) pde_force_lookup(ent); out: diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 1695509370b8..07f75c959173 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -44,7 +44,6 @@ struct proc_dir_entry { const struct proc_ops *proc_ops; const struct file_operations *proc_dir_ops; }; - const struct dentry_operations *proc_dops; union { const struct seq_operations *seq_ops; int (*single_show)(struct seq_file *, void *); @@ -67,6 +66,8 @@ struct proc_dir_entry { char inline_name[]; } __randomize_layout; +#define PROC_ENTRY_FORCE_LOOKUP 2 /* same space as PROC_ENTRY_PERMANENT */ + #define SIZEOF_PDE ( \ sizeof(struct proc_dir_entry) < 128 ? 128 : \ sizeof(struct proc_dir_entry) < 192 ? 192 : \ @@ -346,7 +347,7 @@ extern const struct dentry_operations proc_net_dentry_ops; static inline void pde_force_lookup(struct proc_dir_entry *pde) { /* /proc/net/ entries can be changed under us by setns(CLONE_NEWNET) */ - pde->proc_dops = &proc_net_dentry_ops; + pde->flags |= PROC_ENTRY_FORCE_LOOKUP; } /*
It has two possible values - one for "forced lookup" entries, another for the normal ones. We'd be better off with that as an explicit flag anyway and in addition to that it opens some fun possibilities with ->d_op and ->d_flags handling. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/proc/generic.c | 8 +++++--- fs/proc/internal.h | 5 +++-- 2 files changed, 8 insertions(+), 5 deletions(-)