| Message ID | 20240424233859.7640-1-linux@osuchow.ski (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: Create anon_inode_getfile_fmode() | expand |
On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote: > Creates an anon_inode_getfile_fmode() function that works similarly to > anon_inode_getfile() with the addition of being able to set the fmode > member. And for what use-case exactly?
On 4/25/24 11:57, Christian Brauner wrote: > On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote: >> Creates an anon_inode_getfile_fmode() function that works similarly to >> anon_inode_getfile() with the addition of being able to set the fmode >> member. > > And for what use-case exactly? There was a TODO in the vfio driver [1] to create an interface here that would also allow to set fmode, hence this change. -- Dawid [1] https://lore.kernel.org/kvm/20240424234147.7840-1-linux@osuchow.ski/T/#u
On Thu, Apr 25, 2024 at 11:57:12AM +0200, Christian Brauner wrote: > On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote: > > Creates an anon_inode_getfile_fmode() function that works similarly to > > anon_inode_getfile() with the addition of being able to set the fmode > > member. > > And for what use-case exactly? There are several places where we might want that - arch/powerpc/platforms/pseries/papr-vpd.c:488: file = anon_inode_getfile("[papr-vpd]", &papr_vpd_handle_ops, fs/cachefiles/ondemand.c:233: file = anon_inode_getfile("[cachefiles]", &cachefiles_ondemand_fd_fops, fs/eventfd.c:412: file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx, flags); in addition to vfio example Dawid mentions, as well as a couple of borderline cases in virt/kvm/kvm_main.c:4404: file = anon_inode_getfile(name, &kvm_vcpu_stats_fops, vcpu, O_RDONLY); virt/kvm/kvm_main.c:5092: file = anon_inode_getfile("kvm-vm-stats", So something of that sort is probably a good idea. Said that, what the hell is __anon_inode_getfile_fmode() for? It's identical to exported variant, AFAICS. And then there's this: if (IS_ERR(file)) goto err; file->f_mode |= f_mode; return file; err: return file; a really odd way to spell if (!IS_ERR(file)) file->f_mode |= f_mode; return file;
On 4/25/24 22:25, Al Viro wrote: > On Thu, Apr 25, 2024 at 11:57:12AM +0200, Christian Brauner wrote: >> On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote: >>> Creates an anon_inode_getfile_fmode() function that works similarly to >>> anon_inode_getfile() with the addition of being able to set the fmode >>> member. >> >> And for what use-case exactly? > > There are several places where we might want that - > arch/powerpc/platforms/pseries/papr-vpd.c:488: file = anon_inode_getfile("[papr-vpd]", &papr_vpd_handle_ops, > fs/cachefiles/ondemand.c:233: file = anon_inode_getfile("[cachefiles]", &cachefiles_ondemand_fd_fops, > fs/eventfd.c:412: file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx, flags); > in addition to vfio example Dawid mentions, as well as a couple of > borderline cases in > virt/kvm/kvm_main.c:4404: file = anon_inode_getfile(name, &kvm_vcpu_stats_fops, vcpu, O_RDONLY); > virt/kvm/kvm_main.c:5092: file = anon_inode_getfile("kvm-vm-stats", > > So something of that sort is probably a good idea. Said that, > what the hell is __anon_inode_getfile_fmode() for? It's identical > to exported variant, AFAICS. Thank you for the feedback. I tried emulating what I saw in the other functions (which was clearly wrong). Will address this is in next revision. And then there's this: > > if (IS_ERR(file)) > goto err; > > file->f_mode |= f_mode; > > return file; > > err: > return file; > > a really odd way to spell > > if (!IS_ERR(file)) > file->f_mode |= f_mode; > return file; Same for this, will rewrite it based on your suggestion. -- Dawid
On Thu, Apr 25, 2024 at 09:25:50PM +0100, Al Viro wrote: > On Thu, Apr 25, 2024 at 11:57:12AM +0200, Christian Brauner wrote: > > On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote: > > > Creates an anon_inode_getfile_fmode() function that works similarly to > > > anon_inode_getfile() with the addition of being able to set the fmode > > > member. > > > > And for what use-case exactly? > > There are several places where we might want that - > arch/powerpc/platforms/pseries/papr-vpd.c:488: file = anon_inode_getfile("[papr-vpd]", &papr_vpd_handle_ops, > fs/cachefiles/ondemand.c:233: file = anon_inode_getfile("[cachefiles]", &cachefiles_ondemand_fd_fops, > fs/eventfd.c:412: file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx, flags); > in addition to vfio example Dawid mentions, as well as a couple of > borderline cases in > virt/kvm/kvm_main.c:4404: file = anon_inode_getfile(name, &kvm_vcpu_stats_fops, vcpu, O_RDONLY); > virt/kvm/kvm_main.c:5092: file = anon_inode_getfile("kvm-vm-stats", > > So something of that sort is probably a good idea. Said that, Ok. It wouldn't be the worst if @Dawid would also sent a follow-patch converting the obvious cases over to the new helper then. > what the hell is __anon_inode_getfile_fmode() for? It's identical Unrelated to this patch but the other really unpleasant part is that "make_secure" boolean argument to __anon_inode_getfile() to indicate allocation of a new inode. That "context_inode" argument is also really unused for the normal case where the same anon_inode_inode is reused... IMHO, that should just be split apart. A little more code but it would make things easier to understand imho... > to exported variant, AFAICS. And then there's this: > > if (IS_ERR(file)) > goto err; > > file->f_mode |= f_mode; > > return file; > > err: > return file; > > a really odd way to spell > > if (!IS_ERR(file)) > file->f_mode |= f_mode; > return file; Yeah.
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c index 0496cb5b6eab..6d61d7d1669a 100644 --- a/fs/anon_inodes.c +++ b/fs/anon_inodes.c @@ -148,6 +148,53 @@ struct file *anon_inode_getfile(const char *name, } EXPORT_SYMBOL_GPL(anon_inode_getfile); + +static struct file *__anon_inode_getfile_fmode(const char *name, + const struct file_operations *fops, + void *priv, int flags, fmode_t f_mode) +{ + + struct file *file; + + file = __anon_inode_getfile(name, fops, priv, flags, NULL, false); + if (IS_ERR(file)) + goto err; + + file->f_mode |= f_mode; + + return file; + +err: + return file; +} + +/** + * anon_inode_getfile_fmode - creates a new file instance by hooking it up to an + * anonymous inode, and a dentry that describe the "class" + * of the file + * + * @name: [in] name of the "class" of the new file + * @fops: [in] file operations for the new file + * @priv: [in] private data for the new file (will be file's private_data) + * @flags: [in] flags + * @f_mode: [in] fmode + * + * Creates a new file by hooking it on a single inode. This is useful for files + * that do not need to have a full-fledged inode in order to operate correctly. + * All the files created with anon_inode_getfile() will share a single inode, + * hence saving memory and avoiding code duplication for the file/inode/dentry + * setup. Allows setting the fmode. Returns the newly created file* or an error + * pointer. + */ +struct file *anon_inode_getfile_fmode(const char *name, + const struct file_operations *fops, + void *priv, int flags, fmode_t f_mode) +{ + return __anon_inode_getfile_fmode(name, fops, priv, + flags, f_mode); +} +EXPORT_SYMBOL_GPL(anon_inode_getfile_fmode); + /** * anon_inode_create_getfile - Like anon_inode_getfile(), but creates a new * !S_PRIVATE anon inode rather than reuse the @@ -271,6 +318,7 @@ int anon_inode_create_getfd(const char *name, const struct file_operations *fops return __anon_inode_getfd(name, fops, priv, flags, context_inode, true); } + static int __init anon_inode_init(void) { anon_inode_mnt = kern_mount(&anon_inode_fs_type); diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h index 93a5f16d03f3..ee55f9c11a16 100644 --- a/include/linux/anon_inodes.h +++ b/include/linux/anon_inodes.h @@ -15,6 +15,9 @@ struct inode; struct file *anon_inode_getfile(const char *name, const struct file_operations *fops, void *priv, int flags); +struct file *anon_inode_getfile_fmode(const char *name, + const struct file_operations *fops, + void *priv, int flags, unsigned int f_mode); struct file *anon_inode_create_getfile(const char *name, const struct file_operations *fops, void *priv, int flags,
Creates an anon_inode_getfile_fmode() function that works similarly to anon_inode_getfile() with the addition of being able to set the fmode member. Signed-off-by: Dawid Osuchowski <linux@osuchow.ski> --- fs/anon_inodes.c | 48 +++++++++++++++++++++++++++++++++++++ include/linux/anon_inodes.h | 3 +++ 2 files changed, 51 insertions(+)