| Message ID | 87blpt9e6m.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] proc: Rename in proc_inode rename sysctl_inodes sibling_inodes | expand |
On Thu, Feb 20, 2020 at 12:51 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > To use d_invalidate replace d_prune_aliases with d_find_alias > followed by d_invalidate and dput. This is safe and complete > because no inode in proc has any hardlinks or aliases. Are you sure you can't create them some way? This makes em go "what if we had multiple dentries associated with that inode?" Then the code would just invalidate the first one. I guess we don't have export_operations or anything like that, but this makes me worry... Linus
On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote: > > The function d_prune_aliases has the problem that it will only prune > aliases thare are completely unused. It will not remove aliases for > the dcache or even think of removing mounts from the dcache. For that > behavior d_invalidate is needed. > > To use d_invalidate replace d_prune_aliases with d_find_alias > followed by d_invalidate and dput. This is safe and complete > because no inode in proc has any hardlinks or aliases. s/no inode.*/it's a fucking directory inode./
On Thu, Feb 20, 2020 at 2:54 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > s/no inode.*/it's a directory inode./ That actually makes my worry go away too. We don't allow aliases for directory inodes, iirc. So then it doesn't depend on some /proc implementation issue any more, then it's fundamental that there's only one dentry. Linus
On Thu, Feb 20, 2020 at 10:54:20PM +0000, Al Viro wrote: > On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote: > > > > The function d_prune_aliases has the problem that it will only prune > > aliases thare are completely unused. It will not remove aliases for > > the dcache or even think of removing mounts from the dcache. For that > > behavior d_invalidate is needed. > > > > To use d_invalidate replace d_prune_aliases with d_find_alias > > followed by d_invalidate and dput. This is safe and complete > > because no inode in proc has any hardlinks or aliases. > > s/no inode.*/it's a fucking directory inode./ Wait... You are using it for sysctls as well? Ho-hum... The thing is, for sysctls you are likely to run into consequent entries with the same superblock, making for a big pile of useless playing with ->s_active... And yes, that applied to mainline as well
Al Viro <viro@zeniv.linux.org.uk> writes: > On Thu, Feb 20, 2020 at 10:54:20PM +0000, Al Viro wrote: >> On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote: >> > >> > The function d_prune_aliases has the problem that it will only prune >> > aliases thare are completely unused. It will not remove aliases for >> > the dcache or even think of removing mounts from the dcache. For that >> > behavior d_invalidate is needed. >> > >> > To use d_invalidate replace d_prune_aliases with d_find_alias >> > followed by d_invalidate and dput. This is safe and complete >> > because no inode in proc has any hardlinks or aliases. >> >> s/no inode.*/it's a fucking directory inode./ > > Wait... You are using it for sysctls as well? Ho-hum... The thing is, > for sysctls you are likely to run into consequent entries with the > same superblock, making for a big pile of useless playing with > ->s_active... And yes, that applied to mainline as well Which is why I worked to merge the two cases since they were so close. Fewer things to fix and more eyeballs on the code. Eric
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 38a7baa41aba..c4528c419876 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -103,12 +103,13 @@ void __init proc_init_kmemcache(void) BUILD_BUG_ON(sizeof(struct proc_dir_entry) >= SIZEOF_PDE); } -void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) +void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) { struct inode *inode; struct proc_inode *ei; struct hlist_node *node; struct super_block *sb; + struct dentry *dentry; for (;;) { rcu_read_lock(); @@ -135,7 +136,11 @@ void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock) continue; } - d_prune_aliases(inode); + dentry = d_find_alias(inode); + if (dentry) { + d_invalidate(dentry); + dput(dentry); + } iput(inode); deactivate_super(sb); } diff --git a/fs/proc/internal.h b/fs/proc/internal.h index ba9a991824a5..fd470172675f 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -210,7 +210,7 @@ extern const struct inode_operations proc_pid_link_inode_operations; extern const struct super_operations proc_sops; void proc_init_kmemcache(void); -void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock); +void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock); void set_proc_pid_nlink(void); extern struct inode *proc_get_inode(struct super_block *, struct proc_dir_entry *); extern void proc_entry_rundown(struct proc_dir_entry *); diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 5da9d7f7ae34..b6f5d459b087 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -267,9 +267,9 @@ static void unuse_table(struct ctl_table_header *p) complete(p->unregistering); } -static void proc_sys_prune_dcache(struct ctl_table_header *head) +static void proc_sys_invalidate_dcache(struct ctl_table_header *head) { - proc_prune_siblings_dcache(&head->inodes, &sysctl_lock); + proc_invalidate_siblings_dcache(&head->inodes, &sysctl_lock); } /* called under sysctl_lock, will reacquire if has to wait */ @@ -291,10 +291,10 @@ static void start_unregistering(struct ctl_table_header *p) spin_unlock(&sysctl_lock); } /* - * Prune dentries for unregistered sysctls: namespaced sysctls + * Invalidate dentries for unregistered sysctls: namespaced sysctls * can have duplicate names and contaminate dcache very badly. */ - proc_sys_prune_dcache(p); + proc_sys_invalidate_dcache(p); /* * do not remove from the list until nobody holds it; walking the * list in do_sysctl() relies on that.
The function d_prune_aliases has the problem that it will only prune aliases thare are completely unused. It will not remove aliases for the dcache or even think of removing mounts from the dcache. For that behavior d_invalidate is needed. To use d_invalidate replace d_prune_aliases with d_find_alias followed by d_invalidate and dput. This is safe and complete because no inode in proc has any hardlinks or aliases. To make this behavior change clear rename proc_prune_siblings_dache proc_invalidate_siblings_dcache, and rename proc_sys_prune_dcache proc_sys_invalidate_dcache. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/proc/inode.c | 9 +++++++-- fs/proc/internal.h | 2 +- fs/proc/proc_sysctl.c | 8 ++++---- 3 files changed, 12 insertions(+), 7 deletions(-)