Message ID | 14a09477f0b2d62a424b44e0a1f12f32ae3409bb.1618527437.git.repnop@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | fanotify: Adding pidfd support to the fanotify API | expand |
On Fri 16-04-21 09:22:09, Matthew Bobrowski wrote: > With the idea to have fanotify(7) return pidfds within a `struct > fanotify_event_metadata`, pidfd_create()'s scope is to increased so > that it can be called from other subsystems within the Linux > kernel. The current `static` qualifier from its definition is to be > removed and a new function declaration for pidfd_create() is to be > added to the linux/pid.h header file. > > Signed-off-by: Matthew Bobrowski <repnop@google.com> I'm fine with this but it would be good to get explicit Christian's ack on this patch (I know he's already agreed with the intent in general). Honza > --- > include/linux/pid.h | 1 + > kernel/pid.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/pid.h b/include/linux/pid.h > index fa10acb8d6a4..af308e15f174 100644 > --- a/include/linux/pid.h > +++ b/include/linux/pid.h > @@ -78,6 +78,7 @@ struct file; > > extern struct pid *pidfd_pid(const struct file *file); > struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); > +int pidfd_create(struct pid *pid, unsigned int flags); > > static inline struct pid *get_pid(struct pid *pid) > { > diff --git a/kernel/pid.c b/kernel/pid.c > index ebdf9c60cd0b..91c4b6891c15 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) > * Return: On success, a cloexec pidfd is returned. > * On error, a negative errno number will be returned. > */ > -static int pidfd_create(struct pid *pid, unsigned int flags) > +int pidfd_create(struct pid *pid, unsigned int flags) > { > int fd; > > -- > 2.31.1.368.gbe11c130af-goog > > /M
On Fri, Apr 16, 2021 at 09:22:09AM +1000, Matthew Bobrowski wrote: > With the idea to have fanotify(7) return pidfds within a `struct > fanotify_event_metadata`, pidfd_create()'s scope is to increased so > that it can be called from other subsystems within the Linux > kernel. The current `static` qualifier from its definition is to be > removed and a new function declaration for pidfd_create() is to be > added to the linux/pid.h header file. > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > --- > include/linux/pid.h | 1 + > kernel/pid.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/pid.h b/include/linux/pid.h > index fa10acb8d6a4..af308e15f174 100644 > --- a/include/linux/pid.h > +++ b/include/linux/pid.h > @@ -78,6 +78,7 @@ struct file; > > extern struct pid *pidfd_pid(const struct file *file); > struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); > +int pidfd_create(struct pid *pid, unsigned int flags); > > static inline struct pid *get_pid(struct pid *pid) > { > diff --git a/kernel/pid.c b/kernel/pid.c > index ebdf9c60cd0b..91c4b6891c15 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) > * Return: On success, a cloexec pidfd is returned. > * On error, a negative errno number will be returned. > */ > -static int pidfd_create(struct pid *pid, unsigned int flags) > +int pidfd_create(struct pid *pid, unsigned int flags) Can you please add a comment to the kernel doc mentioning that this helper is _not_ intended to be exported to modules? I don't want drivers to get the idea that it's ok to start returning pidfds from everywhere just yet. And I think we should add sm like if (flags & ~(O_NONBLOCK | O_CLOEXEC | O_RDWR)) return -EINVAL; in pidfd_open() so future callers don't accidently create pidfds with random flags we don't support. Christian
On Mon, Apr 19, 2021 at 02:50:33PM +0200, Christian Brauner wrote: > On Fri, Apr 16, 2021 at 09:22:09AM +1000, Matthew Bobrowski wrote: > > With the idea to have fanotify(7) return pidfds within a `struct > > fanotify_event_metadata`, pidfd_create()'s scope is to increased so > > that it can be called from other subsystems within the Linux > > kernel. The current `static` qualifier from its definition is to be > > removed and a new function declaration for pidfd_create() is to be > > added to the linux/pid.h header file. > > > > Signed-off-by: Matthew Bobrowski <repnop@google.com> > > --- > > include/linux/pid.h | 1 + > > kernel/pid.c | 2 +- > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/pid.h b/include/linux/pid.h > > index fa10acb8d6a4..af308e15f174 100644 > > --- a/include/linux/pid.h > > +++ b/include/linux/pid.h > > @@ -78,6 +78,7 @@ struct file; > > > > extern struct pid *pidfd_pid(const struct file *file); > > struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); > > +int pidfd_create(struct pid *pid, unsigned int flags); > > > > static inline struct pid *get_pid(struct pid *pid) > > { > > diff --git a/kernel/pid.c b/kernel/pid.c > > index ebdf9c60cd0b..91c4b6891c15 100644 > > --- a/kernel/pid.c > > +++ b/kernel/pid.c > > @@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) > > * Return: On success, a cloexec pidfd is returned. > > * On error, a negative errno number will be returned. > > */ > > -static int pidfd_create(struct pid *pid, unsigned int flags) > > > +int pidfd_create(struct pid *pid, unsigned int flags) > > Can you please add a comment to the kernel doc mentioning that this > helper is _not_ intended to be exported to modules? I don't want drivers > to get the idea that it's ok to start returning pidfds from everywhere > just yet. Sure, this makes sense to me. > And I think we should add sm like > > if (flags & ~(O_NONBLOCK | O_CLOEXEC | O_RDWR)) > return -EINVAL; > > in pidfd_open() so future callers don't accidently create pidfds with > random flags we don't support. In the context of exporting pidfd_create() to the rest of the kernel, presumably we should be adding this flag check to pidfd_create() and not pidfd_open(), right? I gather that's what you actually meant. /M
diff --git a/include/linux/pid.h b/include/linux/pid.h index fa10acb8d6a4..af308e15f174 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -78,6 +78,7 @@ struct file; extern struct pid *pidfd_pid(const struct file *file); struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); +int pidfd_create(struct pid *pid, unsigned int flags); static inline struct pid *get_pid(struct pid *pid) { diff --git a/kernel/pid.c b/kernel/pid.c index ebdf9c60cd0b..91c4b6891c15 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) * Return: On success, a cloexec pidfd is returned. * On error, a negative errno number will be returned. */ -static int pidfd_create(struct pid *pid, unsigned int flags) +int pidfd_create(struct pid *pid, unsigned int flags) { int fd;
With the idea to have fanotify(7) return pidfds within a `struct fanotify_event_metadata`, pidfd_create()'s scope is to increased so that it can be called from other subsystems within the Linux kernel. The current `static` qualifier from its definition is to be removed and a new function declaration for pidfd_create() is to be added to the linux/pid.h header file. Signed-off-by: Matthew Bobrowski <repnop@google.com> --- include/linux/pid.h | 1 + kernel/pid.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)