| Message ID | 20250108154338.1129069-28-mic@digikod.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Landlock audit support | expand |
Al, Christian, this standalone patch could be useful to others. Feel free to pick it in your tree. On Wed, Jan 08, 2025 at 04:43:35PM +0100, Mickaël Salaün wrote: > Add a simple scope-based helper to put an inode reference, similar to > the fput() helper. > > This is used in a following commit. > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jeff Layton <jlayton@kernel.org> > Cc: Josef Bacik <josef@toxicpanda.com> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Link: https://lore.kernel.org/r/20250108154338.1129069-28-mic@digikod.net > --- > > Changes since v3: > - New patch. > --- > include/linux/fs.h | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 7e29433c5ecc..bd5a28b0871f 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -47,6 +47,8 @@ > #include <linux/rw_hint.h> > #include <linux/file_ref.h> > #include <linux/unicode.h> > +#include <linux/cleanup.h> > +#include <linux/err.h> > > #include <asm/byteorder.h> > #include <uapi/linux/fs.h> > @@ -2698,6 +2700,8 @@ extern void iput(struct inode *); > int inode_update_timestamps(struct inode *inode, int flags); > int generic_update_time(struct inode *, int); > > +DEFINE_FREE(iput, struct inode *, if (!IS_ERR_OR_NULL(_T)) iput(_T)) > + > /* /sys/fs */ > extern struct kobject *fs_kobj; > > @@ -3108,8 +3112,6 @@ static inline bool is_dot_dotdot(const char *name, size_t len) > (len == 1 || (len == 2 && name[1] == '.')); > } > > -#include <linux/err.h> > - > /* needed for stackable file system support */ > extern loff_t default_llseek(struct file *file, loff_t offset, int whence); > > -- > 2.47.1 > >
On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün <mic@digikod.net> wrote: > Add a simple scope-based helper to put an inode reference, similar to > the fput() helper. Cleaning up inode references with scope-based cleanup seems dangerous to me because, unlike most resources, holding a reference to an inode beyond the lifetime of the associated superblock can actually cause memory corruption; and scope-based cleanup is designed based on the idea that the order and precise location of dropping a reference don't matter so much. So I would prefer to either not do this, or at least have some big warning comment about usage requirements of scope-based inode references.
On Wed, 08 Jan 2025 16:43:35 +0100, Mickaël Salaün wrote: > Add a simple scope-based helper to put an inode reference, similar to > the fput() helper. > > This is used in a following commit. > > Applied to the vfs-6.14.misc branch of the vfs/vfs.git tree. Patches in the vfs-6.14.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.14.misc [27/30] fs: Add iput() cleanup helper https://git.kernel.org/vfs/vfs/c/38b1ff0bcff1
On Mon, Jan 13, 2025 at 03:00:20PM +0100, Jann Horn wrote: > On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün <mic@digikod.net> wrote: > > Add a simple scope-based helper to put an inode reference, similar to > > the fput() helper. > > Cleaning up inode references with scope-based cleanup seems dangerous > to me because, unlike most resources, holding a reference to an inode > beyond the lifetime of the associated superblock can actually cause > memory corruption; and scope-based cleanup is designed based on the > idea that the order and precise location of dropping a reference don't > matter so much. That's in general a good point and I know there's been opposition to this in the past when we discussed this. So fine by me.
On Mon, Jan 13, 2025 at 12:15:05PM +0100, Mickaël Salaün wrote: > Al, Christian, this standalone patch could be useful to others. Feel > free to pick it in your tree. Bad idea - we'll end up having to treat uses of that as a serious red flag; it's too easy to end up with lifetime extended too far.
On Mon, Jan 13, 2025 at 04:00:31PM +0100, Christian Brauner wrote: > On Mon, Jan 13, 2025 at 03:00:20PM +0100, Jann Horn wrote: > > On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün <mic@digikod.net> wrote: > > > Add a simple scope-based helper to put an inode reference, similar to > > > the fput() helper. > > > > Cleaning up inode references with scope-based cleanup seems dangerous > > to me because, unlike most resources, holding a reference to an inode > > beyond the lifetime of the associated superblock can actually cause > > memory corruption; and scope-based cleanup is designed based on the > > idea that the order and precise location of dropping a reference don't > > matter so much. > > That's in general a good point and I know there's been opposition to > this in the past when we discussed this. So fine by me. Right, I'll use __free(path_put) instead.
diff --git a/include/linux/fs.h b/include/linux/fs.h index 7e29433c5ecc..bd5a28b0871f 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -47,6 +47,8 @@ #include <linux/rw_hint.h> #include <linux/file_ref.h> #include <linux/unicode.h> +#include <linux/cleanup.h> +#include <linux/err.h> #include <asm/byteorder.h> #include <uapi/linux/fs.h> @@ -2698,6 +2700,8 @@ extern void iput(struct inode *); int inode_update_timestamps(struct inode *inode, int flags); int generic_update_time(struct inode *, int); +DEFINE_FREE(iput, struct inode *, if (!IS_ERR_OR_NULL(_T)) iput(_T)) + /* /sys/fs */ extern struct kobject *fs_kobj; @@ -3108,8 +3112,6 @@ static inline bool is_dot_dotdot(const char *name, size_t len) (len == 1 || (len == 2 && name[1] == '.')); } -#include <linux/err.h> - /* needed for stackable file system support */ extern loff_t default_llseek(struct file *file, loff_t offset, int whence);
Add a simple scope-based helper to put an inode reference, similar to the fput() helper. This is used in a following commit. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jeff Layton <jlayton@kernel.org> Cc: Josef Bacik <josef@toxicpanda.com> Signed-off-by: Mickaël Salaün <mic@digikod.net> Link: https://lore.kernel.org/r/20250108154338.1129069-28-mic@digikod.net --- Changes since v3: - New patch. --- include/linux/fs.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)