Re: [1/1] Use pid in inotify events.

Re: [1/1] Use pid in inotify events.

[Evgeniy Polyakov]

On Sat, Nov 08, 2008 at 01:35:07PM -0500, Robert Love (rlove@rlove.org) wrote:
> Pointing out some other information leak doesn't lessen the pid problem.

I just wanted to show that this is not a security problem.
Since there are others probably even more important.

But I really do not want to start to discuss this, since I already see
that result will be effectively zero, so returning to the original
problem with putting pid into events: if inotify_init() caller has 0
(e)uid or if IO origin has the same (e)uid, are you ok to put pid into
the event?

-- 
	Evgeniy Polyakov

Re: [1/1] Use pid in inotify events.

[Evgeniy Polyakov]

On Sat, Nov 08, 2008 at 09:40:13PM +0300, Evgeniy Polyakov (zbr@ioremap.net) wrote:
> But I really do not want to start to discuss this, since I already see
> that result will be effectively zero, so returning to the original
> problem with putting pid into events: if inotify_init() caller has 0
> (e)uid or if IO origin has the same (e)uid, are you ok to put pid into
> the event?

Something like this (not tested yet, no normal network access).

Signed-off.

diff --git a/fs/inotify.c b/fs/inotify.c
index 690e725..121f025 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -80,6 +80,7 @@ struct inotify_handle {
 	struct list_head	watches;	/* list of watches */
 	atomic_t		count;		/* reference count */
 	u32			last_wd;	/* the last wd allocated */
+	uid_t			uid;		/* watcher's uid */
 	const struct inotify_operations *in_ops; /* inotify caller operations */
 };
 
@@ -292,6 +293,10 @@ void inotify_inode_queue_event(struct inode *inode, u32 mask, u32 cookie,
 			mutex_lock(&ih->mutex);
 			if (watch_mask & IN_ONESHOT)
 				remove_watch_no_event(watch, ih);
+
+			if (!cookie && (ih->uid == 0 ||
+					ih->uid == current->user->uid))
+				cookie = task_tgid_vnr(current);
 			ih->in_ops->handle_event(watch, watch->wd, mask, cookie,
 						 name, n_inode);
 			mutex_unlock(&ih->mutex);
diff --git a/fs/inotify_user.c b/fs/inotify_user.c
index 6024942..e426f7c 100644
--- a/fs/inotify_user.c
+++ b/fs/inotify_user.c
@@ -612,6 +612,8 @@ asmlinkage long sys_inotify_init1(int flags)
 	dev->ih = ih;
 	dev->fa = NULL;
 
+	ih->uid = user->uid;
+
 	filp->f_op = &inotify_fops;
 	filp->f_path.mnt = mntget(inotify_mnt);
 	filp->f_path.dentry = dget(inotify_mnt->mnt_root);


-- 
	Evgeniy Polyakov

[take 2] Use pid in inotify events.

[Evgeniy Polyakov]

Hi.

This patch allows to send IO origin PID in inotify events (using cookie
fields for all events except moving, where it is already used to track
move from and move to parts) when its uid matches inotify owner uid or
when inotify owner has admin capabilities (Jeff Schroeder's idea).

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>

diff --git a/fs/inotify.c b/fs/inotify.c
index 690e725..835259d 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -69,6 +69,9 @@ static atomic_t inotify_cookie;
  * inotify_add_watch() to the final put_inotify_watch().
  */
 
+#define IH_FLAGS_ADMIN		(0x00000001)
+/* handler owner has admin capabilities */
+
 /*
  * struct inotify_handle - represents an inotify instance
  *
@@ -80,6 +83,8 @@ struct inotify_handle {
 	struct list_head	watches;	/* list of watches */
 	atomic_t		count;		/* reference count */
 	u32			last_wd;	/* the last wd allocated */
+	uid_t			uid;		/* owner's uid */
+	u32			flags;		/* operation flags */
 	const struct inotify_operations *in_ops; /* inotify caller operations */
 };
 
@@ -292,6 +297,11 @@ void inotify_inode_queue_event(struct inode *inode, u32 mask, u32 cookie,
 			mutex_lock(&ih->mutex);
 			if (watch_mask & IN_ONESHOT)
 				remove_watch_no_event(watch, ih);
+
+			if (!cookie && ((ih->flags & IH_FLAGS_ADMIN) ||
+					(current->uid == ih->uid)))
+				cookie = task_tgid_vnr(current);
+
 			ih->in_ops->handle_event(watch, watch->wd, mask, cookie,
 						 name, n_inode);
 			mutex_unlock(&ih->mutex);
@@ -459,6 +469,10 @@ struct inotify_handle *inotify_init(const struct inotify_operations *ops)
 	mutex_init(&ih->mutex);
 	ih->last_wd = 0;
 	ih->in_ops = ops;
+	ih->uid = current->user->uid;
+	ih->flags = 0;
+	if (capable(CAP_SYS_ADMIN))
+		ih->flags |= IH_FLAGS_ADMIN;
 	atomic_set(&ih->count, 0);
 	get_inotify_handle(ih);
 
-- 
	Evgeniy Polyakov

[take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi.

This patch allows to send IO origin PID in inotify events (using cookie
fields for all events except moving, where it is already used to track
move from and move to parts) when its uid matches inotify owner uid or
when inotify owner has admin (CAP_SYS_ADMIN) capabilities (Jeff
Schroeder's idea).

This is a resend of the previous patch, which was not commented by
anyone. Does it mean no one objects? If so, please apply.

Also removed John McCutchan's email, which bounces.

Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>

diff --git a/fs/inotify.c b/fs/inotify.c
index 690e725..835259d 100644
--- a/fs/inotify.c
+++ b/fs/inotify.c
@@ -69,6 +69,9 @@ static atomic_t inotify_cookie;
  * inotify_add_watch() to the final put_inotify_watch().
  */
 
+#define IH_FLAGS_ADMIN		(0x00000001)
+/* handler owner has admin capabilities */
+
 /*
  * struct inotify_handle - represents an inotify instance
  *
@@ -80,6 +83,8 @@ struct inotify_handle {
 	struct list_head	watches;	/* list of watches */
 	atomic_t		count;		/* reference count */
 	u32			last_wd;	/* the last wd allocated */
+	uid_t			uid;		/* owner's uid */
+	u32			flags;		/* operation flags */
 	const struct inotify_operations *in_ops; /* inotify caller operations */
 };
 
@@ -292,6 +297,11 @@ void inotify_inode_queue_event(struct inode *inode, u32 mask, u32 cookie,
 			mutex_lock(&ih->mutex);
 			if (watch_mask & IN_ONESHOT)
 				remove_watch_no_event(watch, ih);
+
+			if (!cookie && ((ih->flags & IH_FLAGS_ADMIN) ||
+					(current->uid == ih->uid)))
+				cookie = task_tgid_vnr(current);
+
 			ih->in_ops->handle_event(watch, watch->wd, mask, cookie,
 						 name, n_inode);
 			mutex_unlock(&ih->mutex);
@@ -459,6 +469,10 @@ struct inotify_handle *inotify_init(const struct inotify_operations *ops)
 	mutex_init(&ih->mutex);
 	ih->last_wd = 0;
 	ih->in_ops = ops;
+	ih->uid = current->user->uid;
+	ih->flags = 0;
+	if (capable(CAP_SYS_ADMIN))
+		ih->flags |= IH_FLAGS_ADMIN;
 	atomic_set(&ih->count, 0);
 	get_inotify_handle(ih);
 
-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Michael Kerrisk]

Hi Evgeniy,

On Sun, Nov 16, 2008 at 6:24 PM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> Hi.
>
> This patch allows to send IO origin PID in inotify events (using cookie
> fields for all events except moving, where it is already used to track
> move from and move to parts) when its uid matches inotify owner uid or
> when inotify owner has admin (CAP_SYS_ADMIN) capabilities (Jeff
> Schroeder's idea).
>
> This is a resend of the previous patch, which was not commented by
> anyone. Does it mean no one objects? If so, please apply.

NAK.  If we are going to do this -- and I leave the security
discussions to others more knowlegeable on that score than me -- then
the API design should be better than this.  The current design is a
hack.  Why exclude rename events?  Why re-use the cookie field?  The
only answers I can guess at are that the current patch is less work to
write.  IMO, there are (much) better design possibilities, using
inotify1(), as I suggested earlier in this thread.

Thanks,

Michael

> Also removed John McCutchan's email, which bounces.
>
> Signed-off-by: Evgeniy Polyakov <zbr@ioremap.net>
>
> diff --git a/fs/inotify.c b/fs/inotify.c
> index 690e725..835259d 100644
> --- a/fs/inotify.c
> +++ b/fs/inotify.c
> @@ -69,6 +69,9 @@ static atomic_t inotify_cookie;
>  * inotify_add_watch() to the final put_inotify_watch().
>  */
>
> +#define IH_FLAGS_ADMIN         (0x00000001)
> +/* handler owner has admin capabilities */
> +
>  /*
>  * struct inotify_handle - represents an inotify instance
>  *
> @@ -80,6 +83,8 @@ struct inotify_handle {
>        struct list_head        watches;        /* list of watches */
>        atomic_t                count;          /* reference count */
>        u32                     last_wd;        /* the last wd allocated */
> +       uid_t                   uid;            /* owner's uid */
> +       u32                     flags;          /* operation flags */
>        const struct inotify_operations *in_ops; /* inotify caller operations */
>  };
>
> @@ -292,6 +297,11 @@ void inotify_inode_queue_event(struct inode *inode, u32 mask, u32 cookie,
>                        mutex_lock(&ih->mutex);
>                        if (watch_mask & IN_ONESHOT)
>                                remove_watch_no_event(watch, ih);
> +
> +                       if (!cookie && ((ih->flags & IH_FLAGS_ADMIN) ||
> +                                       (current->uid == ih->uid)))
> +                               cookie = task_tgid_vnr(current);
> +
>                        ih->in_ops->handle_event(watch, watch->wd, mask, cookie,
>                                                 name, n_inode);
>                        mutex_unlock(&ih->mutex);
> @@ -459,6 +469,10 @@ struct inotify_handle *inotify_init(const struct inotify_operations *ops)
>        mutex_init(&ih->mutex);
>        ih->last_wd = 0;
>        ih->in_ops = ops;
> +       ih->uid = current->user->uid;
> +       ih->flags = 0;
> +       if (capable(CAP_SYS_ADMIN))
> +               ih->flags |= IH_FLAGS_ADMIN;
>        atomic_set(&ih->count, 0);
>        get_inotify_handle(ih);
>
> --
>        Evgeniy Polyakov
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi Michael.

On Mon, Nov 17, 2008 at 11:59:11AM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
> NAK.  If we are going to do this -- and I leave the security
> discussions to others more knowlegeable on that score than me -- then
> the API design should be better than this.  The current design is a
> hack.  Why exclude rename events?  Why re-use the cookie field?  The
> only answers I can guess at are that the current patch is less work to
> write.  IMO, there are (much) better design possibilities, using
> inotify1(), as I suggested earlier in this thread.

Cookie was created to store information used to somehow connect events to
each other. PID does that from another angle than rename. Extending
(rewriting userspace event processing part) events is a solution for the
new project, while existing patch (where all security concerns are
resolved) is a minimum functionality extension.

if I will spent a day and rewrite userspace report side to report new
events I'm pretty sure there will be people, who will start complaining
that again design does not match some theoretically perfect
expectations, and for the purpose of reporting origin's PID cookie
fields can be reused since right now it is unused.

Plus, if it is that hard to comment on patch which adds 14 (!) lines
including blank, which feedback we should expect on larger one? :)

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Michael Kerrisk]

Hi Evgeniy,

On Mon, Nov 17, 2008 at 12:15 PM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> Hi Michael.
>
> On Mon, Nov 17, 2008 at 11:59:11AM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
>> NAK.  If we are going to do this -- and I leave the security
>> discussions to others more knowlegeable on that score than me -- then
>> the API design should be better than this.  The current design is a
>> hack.  Why exclude rename events?  Why re-use the cookie field?  The
>> only answers I can guess at are that the current patch is less work to
>> write.  IMO, there are (much) better design possibilities, using
>> inotify1(), as I suggested earlier in this thread.
>
> Cookie was created to store information used to somehow connect events to
> each other. PID does that from another angle than rename.

Yes, but it does it in an inconsistent, incomplete way.

> Extending
> (rewriting userspace event processing part) events is a solution for the
> new project,

Not quite sure of your point here.  Whatever change is made, userspace
apps will need to be trained to understand the interface.

> while existing patch (where all security concerns are
> resolved) is a minimum functionality extension.

It is a minimum functionality extension that serves the needs of one
or a few projects, while dirtying the design for all users.

> if I will spent a day and rewrite userspace report side to report new
> events I'm pretty sure there will be people, who will start complaining
> that again design does not match some theoretically perfect
> expectations,

Maybe.  Mabe not.  But that is (a necessary) part of the design process.

> and for the purpose of reporting origin's PID cookie
> fields can be reused since right now it is unused.

You didn't really respond to my earlier comment.  Why are you doing
things this way.  As far as I can see, only becuase it is quicker to
implement.

> Plus, if it is that hard to comment on patch which adds 14 (!) lines
> including blank, which feedback we should expect on larger one? :)

Still NAK, sorry.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Mon, Nov 17, 2008 at 12:23:01PM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
> > Cookie was created to store information used to somehow connect events to
> > each other. PID does that from another angle than rename.
> 
> Yes, but it does it in an inconsistent, incomplete way.

It was not my decision, I can not argue if it could be good, bad, perfect
or shine. It is what we have, and I'm trying to extend it not breaking
other things up.

> > Extending
> > (rewriting userspace event processing part) events is a solution for the
> > new project,
> 
> Not quite sure of your point here.  Whatever change is made, userspace
> apps will need to be trained to understand the interface.

I mean kernel event generation side will have to be rewritten: new
event structures, new members, new field usage scenario and so on.

> > while existing patch (where all security concerns are
> > resolved) is a minimum functionality extension.
> 
> It is a minimum functionality extension that serves the needs of one
> or a few projects, while dirtying the design for all users.

Yes, this is a minimum functionality extension, which breaks nothing.
That's why it is a good idea, but I agree that there may be better than
just a good idea and implementation :)

Users do not use cookie field, since it is unused by all but two events
(move_to and move_from), now it may carry not zero, but process ID of
the IO origin for all but two events. Exactly the same situation.
Even more on this: because of mismatched uids process will see
the same zero as before for all 'alien' IO, and have a non-zero cookie
to show that IO belongs to the same user as watcher.

> > if I will spent a day and rewrite userspace report side to report new
> > events I'm pretty sure there will be people, who will start complaining
> > that again design does not match some theoretically perfect
> > expectations,
> 
> Maybe.  Mabe not.  But that is (a necessary) part of the design process.

No, this will be done not at design time. At design time it requires to
think about how to implement the feature, when things are done it is
much more comfortable and more pleasure to flame about.
I already messed with generic interfaces 3 years ago :)

That's a joke of course, it is possible that it will be very popular
frequently used interface. We can discuss and create a good interface,
but who will use it (except me, who wants this for the own project)?

> > and for the purpose of reporting origin's PID cookie
> > fields can be reused since right now it is unused.
> 
> You didn't really respond to my earlier comment.  Why are you doing
> things this way.  As far as I can see, only becuase it is quicker to
> implement.

And because it will be/is used. Even current inotify is very rarely used
(it was created to solve particular problem for single application
those days) by similar to beagle programms, do you really expect they
suddenly will jump into the vagon and change the whole interfaces
because of that fact, that new events have pid not in old cookie but in
additional field? That new inotify1() will be used by my single
application only :) While I propose to extend existing interface not
disturbing anyone.

And I actually answered, that this may be a good idea for the new
project. Although if things work right now no one will ever try to
change it. It does not work in my case, so I need to invent as simple
as possible way to fix it.

> > Plus, if it is that hard to comment on patch which adds 14 (!) lines
> > including blank, which feedback we should expect on larger one? :)
> 
> Still NAK, sorry.

That was kind of rhetorical question, I understood that you want to
change interface to something different with cleaner layout :)
Like having pid in a different field, which will be unused for all
events except those originated from the processes with the same UID as
watcher application.

But now I'm curious about feedback you think will be done for the
updated version?

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Pavel Machek]

On Mon 2008-11-17 20:52:12, Evgeniy Polyakov wrote:
> On Mon, Nov 17, 2008 at 12:23:01PM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
> > > Cookie was created to store information used to somehow connect events to
> > > each other. PID does that from another angle than rename.
> > 
> > Yes, but it does it in an inconsistent, incomplete way.
> 
> It was not my decision, I can not argue if it could be good, bad, perfect
> or shine. It is what we have, and I'm trying to extend it not breaking
> other things up.
> 
> > > Extending
> > > (rewriting userspace event processing part) events is a solution for the
> > > new project,
> > 
> > Not quite sure of your point here.  Whatever change is made, userspace
> > apps will need to be trained to understand the interface.
> 
> I mean kernel event generation side will have to be rewritten: new
> event structures, new members, new field usage scenario and so on.
> 
> > > while existing patch (where all security concerns are
> > > resolved) is a minimum functionality extension.
> > 
> > It is a minimum functionality extension that serves the needs of one
> > or a few projects, while dirtying the design for all users.
> 
> Yes, this is a minimum functionality extension, which breaks nothing.
> That's why it is a good idea, but I agree that there may be better than
> just a good idea and implementation :)

Breaks nothing?!

Introducing ugly hack with broken permission check we have to maintain
forever seems like way too much breakage for 14 lines.

> And I actually answered, that this may be a good idea for the new
> project. Although if things work right now no one will ever try to
> change it. It does not work in my case, so I need to invent as simple
> as possible way to fix it.

'as simple diff as possible' is pretty bad criterium for kernel
merges.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Thu, Nov 20, 2008 at 02:09:03PM +0100, Pavel Machek (pavel@suse.cz) wrote:
> > > It is a minimum functionality extension that serves the needs of one
> > > or a few projects, while dirtying the design for all users.
> > 
> > Yes, this is a minimum functionality extension, which breaks nothing.
> > That's why it is a good idea, but I agree that there may be better than
> > just a good idea and implementation :)
> 
> Breaks nothing?!
> 
> Introducing ugly hack with broken permission check we have to maintain
> forever seems like way too much breakage for 14 lines.

Apparently you missed the patch itself.
Please check it first before making such statements.

> > And I actually answered, that this may be a good idea for the new
> > project. Although if things work right now no one will ever try to
> > change it. It does not work in my case, so I need to invent as simple
> > as possible way to fix it.
> 
> 'as simple diff as possible' is pretty bad criterium for kernel
> merges.

Critics without suggestions is useless. What did you try to say here?
You you believe it should be done in a different way, please tell us how
you see this should be implemented.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Michael Kerrisk]

>> > And I actually answered, that this may be a good idea for the new
>> > project. Although if things work right now no one will ever try to
>> > change it. It does not work in my case, so I need to invent as simple
>> > as possible way to fix it.
>>
>> 'as simple diff as possible' is pretty bad criterium for kernel
>> merges.
>
> Critics without suggestions is useless. What did you try to say here?
> You you believe it should be done in a different way, please tell us how
> you see this should be implemented.

Hi Evgeniy,

I think the point is this.  You want to introduce a change that
address the needs of a single application.  Many others see the
change, which though it may be simple and quick to implement, as "an
ugly hack" -- it messes up an otherwise rather well designed API.
Should we make such a change?  I think not -- and others are echoing
that sentiment.  Your argument that "we should do this because no-one
else has proposed a better way" is not sufficient rationale for
uglifying this API to serve the needs of a single app -- the argument
will not fly, no matter how many times you repeat it.  (Your statement
"Critics without suggestions is useless" does not hold: one very
useful purpose of critics is to maintain the status quo of "good
taste" in API design.)

At this stage, I see three possibilities -- you maintain an
out-of-mainline patch for the kernel, and distribute that with your
app; you work out some other *userspace* solution to your problem; or
someone comes up with inotify-ng, designed to address your needs and
those of others (okay, we may not know what those other needs are yet,
but the question is if we could come up with an inotify-ng design that
can extensible in a sane way).  I know that none of these options will
be what you are happy with, but all of them have more life than your
proposal, IMO.

Cheers,

Michael

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi Michael.

On Fri, Nov 21, 2008 at 09:20:39AM -0500, Michael Kerrisk (mtk.manpages@gmail.com) wrote:
> I think the point is this.  You want to introduce a change that
> address the needs of a single application.  Many others see the
> change, which though it may be simple and quick to implement, as "an
> ugly hack" -- it messes up an otherwise rather well designed API.
> Should we make such a change?  I think not -- and others are echoing
> that sentiment.  Your argument that "we should do this because no-one
> else has proposed a better way" is not sufficient rationale for
> uglifying this API to serve the needs of a single app -- the argument
> will not fly, no matter how many times you repeat it.  (Your statement
> "Critics without suggestions is useless" does not hold: one very
> useful purpose of critics is to maintain the status quo of "good
> taste" in API design.)

That's the point, I proposed an idea, people just say no, without
discussion on how this should be implemented. This is a sign that
people do not really know how they want this to be implemented, if
they care at all, but want to show that some cenversation took place.

It does not. There may be infinite ways to satisfy taste of the
beautiful for lots of people, I already tried, so know this perfectly
well. And when I ask how others expect it to look like, I got _zero_
responses except that to put it into different field, when you proposed
new inotify interface.

No one proposed netlink-like attributes nesting, no one proposed new
fields, nothing. Because no one really cares about that. Only becuase of
this fact I'm still trying to say that existing inotify works ok and its
ugly extension is not a bad idea. Because no one needs new inotify, new
fileds and new interfaces.

> At this stage, I see three possibilities -- you maintain an
> out-of-mainline patch for the kernel, and distribute that with your
> app; you work out some other *userspace* solution to your problem; or
> someone comes up with inotify-ng, designed to address your needs and
> those of others (okay, we may not know what those other needs are yet,
> but the question is if we could come up with an inotify-ng design that
> can extensible in a sane way).  I know that none of these options will
> be what you are happy with, but all of them have more life than your
> proposal, IMO.

I'm happy with any solution, which solves the problem. I proposed one.
It was not accepted. So I asked how this should look like? No response.
I proposed some ideas (pid, start/offset of the io) - still no response
if it is good, bad, ugly or beautiful.

But instead people want to throw a stone, that something is ugly.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Robert Love]

On Fri, Nov 21, 2008 at 9:03 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:

> Critics without suggestions is useless. What did you try to say here?
> You you believe it should be done in a different way, please tell us how
> you see this should be implemented.

Pavel has the bedside manner of a T-Rex, but he is right.

Your solution needs to be (a) generally applicable and useful, with an
(b) elegant and clean API, which (c) does not break ABI or API.

Overloading the cookie field is not the way to go. Finding ways to
extend the API through inotify_init might be--you will have even
higher hurdles of "do we really need this" though.

John & I intentionally did not add the pid field when writing inotify
for reasons of security and questionable need. It also stinks to have
to add a pid field to the event structure if that field is seldom
used.

Working on lkml often sounds like everyone is screaming NO, channeling
nothing but stop energy. Sometimes people are, but more often what
they really mean is you just have to take your time and do things
right. Admittedly it is a lot of iteration, but Linux is a noble
pursuit.

     Robert

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Fri, Nov 21, 2008 at 09:30:38AM -0500, Robert Love (rlove@rlove.org) wrote:
> Your solution needs to be (a) generally applicable and useful, with an
> (b) elegant and clean API, which (c) does not break ABI or API.
> 
> Overloading the cookie field is not the way to go. Finding ways to
> extend the API through inotify_init might be--you will have even
> higher hurdles of "do we really need this" though.

That's it. Does it mean neither solution will be accepted?
Just because 'we' do not need to know IO origin identity.

According to your three requirements for the solution. They can not be
satisfied, just because inotify event returned to userspace is fixed, so
there will be at least extension of the API and ABI.

> John & I intentionally did not add the pid field when writing inotify
> for reasons of security and questionable need. It also stinks to have
> to add a pid field to the event structure if that field is seldom
> used.

That's it: overloading existing cookie is a no-go, new interface is not
needed :)

What I would implement if things are getting that far, is a nesting
attributes in form of header and data, like

[generic inotify header: event, watch and attached data size]
[attribute0 size data]
[attribute1 size data]
...
[attributeN size data]

where attribute list, needed to be sent per event is created via ioctls
on top of inotify file descriptor, since overloading flag value of the
inotify_init1() allows to have only 32 attributes, while that may be not
enough. So far I see several: pid, IO offset and start, attributes
changed (access mode, permissions, xattrs names), combine move event
into two attributes of the same event instead of two events with the
same cookie. Maybe something else, this can be extended infinitely.

> Working on lkml often sounds like everyone is screaming NO, channeling
> nothing but stop energy. Sometimes people are, but more often what
> they really mean is you just have to take your time and do things
> right. Admittedly it is a lot of iteration, but Linux is a noble
> pursuit.

It is linux-kernel@ only. All subsystems I worked with behave
cooperately to solve the problem. All except generic changes, which end
up in linux-kernel@. But that's the matter of feeling that this is a
so special mail lists. We can live with it of course :)

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Pavel Machek]

On Fri 2008-11-21 09:30:38, Robert Love wrote:
> On Fri, Nov 21, 2008 at 9:03 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> 
> > Critics without suggestions is useless. What did you try to say here?
> > You you believe it should be done in a different way, please tell us how
> > you see this should be implemented.
> 
> Pavel has the bedside manner of a T-Rex, but he is right.

Heh. Will attach T-Rex to next email.

> Your solution needs to be (a) generally applicable and useful, with an
> (b) elegant and clean API, which (c) does not break ABI or API.
> 
> Overloading the cookie field is not the way to go. Finding ways to
> extend the API through inotify_init might be--you will have even
> higher hurdles of "do we really need this" though.
> 
> John & I intentionally did not add the pid field when writing inotify
> for reasons of security and questionable need. It also stinks to have
> to add a pid field to the event structure if that field is seldom
> used.

...plus the permission check was quite strange. We don't normally try
to hide PIDs, and 'equal uid' is very non-standard test. can_ptrace()
is normally used for such stuff...

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Fri, Nov 21, 2008 at 03:57:09PM +0100, Pavel Machek (pavel@suse.cz) wrote:
> > John & I intentionally did not add the pid field when writing inotify
> > for reasons of security and questionable need. It also stinks to have
> > to add a pid field to the event structure if that field is seldom
> > used.
> 
> ...plus the permission check was quite strange. We don't normally try
> to hide PIDs, and 'equal uid' is very non-standard test. can_ptrace()
> is normally used for such stuff...

That's what Reobert suggested as a security measure. Expect this
decision will not be described in details, why it is good or bad.

I'm prefectly fine without this check either.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Christoph Hellwig]

On Mon, Nov 17, 2008 at 11:59:11AM -0500, Michael Kerrisk wrote:
> NAK.  If we are going to do this -- and I leave the security
> discussions to others more knowlegeable on that score than me -- then
> the API design should be better than this.  The current design is a
> hack.  Why exclude rename events?  Why re-use the cookie field?  The
> only answers I can guess at are that the current patch is less work to
> write.  IMO, there are (much) better design possibilities, using
> inotify1(), as I suggested earlier in this thread.

Yes, this kind of thing should be enable using an flag to inotify1, and
be consistant even for rename.  Doing it as a flag to inotify1 also has
the advantage to be able to return an -EPERM when the feature is
requested but not allowed instead of letting applications that assume it
silently fail.

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi Christoph.

On Tue, Nov 18, 2008 at 02:19:37PM +0100, Christoph Hellwig (hch@lst.de) wrote:
> Yes, this kind of thing should be enable using an flag to inotify1, and
> be consistant even for rename.  Doing it as a flag to inotify1 also has
> the advantage to be able to return an -EPERM when the feature is
> requested but not allowed instead of letting applications that assume it
> silently fail.

So effectively you propose to have second generation of the inotify
which will have additional pid field, which will be unused by all but
the same uid events?

If you want to return -EPERM, than it will be _always_ returned for non
sysadmin capable user, which effectively makes it unusable.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Michael Kerrisk]

[RESENT, because LKML bounced some HTML that accidentally got put in the mail.]
[CC+=John McCutchan, this time with hopefully a live email address;
John, some context here: http://marc.info/?t=122633022400003&r=1&w=2 ]

Evgeniy,

On Wed, Nov 19, 2008 at 9:05 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
>
> Hi Christoph.
>
> On Tue, Nov 18, 2008 at 02:19:37PM +0100, Christoph Hellwig (hch@lst.de) wrote:
> > Yes, this kind of thing should be enable using an flag to inotify1, and
> > be consistant even for rename.  Doing it as a flag to inotify1 also has
> > the advantage to be able to return an -EPERM when the feature is
> > requested but not allowed instead of letting applications that assume it
> > silently fail.
>
> So effectively you propose to have second generation of the inotify
> which will have additional pid field, which will be unused by all but
> the same uid events?

I suspect that Christoph wants the same thing as I do: some thinking
towards a future-proof design, rather than a quick hack to address the
needs of a single application.

> If you want to return -EPERM, than it will be _always_ returned for non
> sysadmin capable user, which effectively makes it unusable.

Again, appropriate flags in inotify_init1() could fix this -- e.g.,
only fill the field (and give an error if no perms) if a flag is set.

I think what is really needed at this point is some consideration of
what other extensions (if any) might be desired for inotify, and how
we might be best create a design that suits those and future needs.

Cheers,

Michael


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [take 3] Use pid in inotify events.

[Ville Syrjälä]

On Wed, Nov 19, 2008 at 09:43:41AM -0500, Michael Kerrisk wrote:
> I think what is really needed at this point is some consideration of
> what other extensions (if any) might be desired for inotify, and how
> we might be best create a design that suits those and future needs.

In case people are taking notes for "inotify ng" I have one improvement
request. Add some way to know when a move event is complete. Currently if
you're just wathing the source or target directory of a move you only get
one of the MOVED_FROM/MOVED_TO events. Not knowing whether you'll
eventually receive the other half of the event is problematic when you
want to treat such unpaired events the same as DELETE/CREATE events.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi Michael.

On Wed, Nov 19, 2008 at 09:34:46AM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
> > So effectively you propose to have second generation of the inotify
> > which will have additional pid field, which will be unused by all but
> > the same uid events?
> 
> I susepect that Christoph wants the same thing as I do: some thinking
> towards a future-proof design, rather than a quick hack to address the needs
> of a single application.

So far the only real need is a pid. That will solve the cases I'm
working on and it may be interesting for other applications. It is
possible to extend read/write IO with offset and size parameters though.

Do you see any other possible extensions?

> > If you want to return -EPERM, than it will be _always_ returned for non
> > sysadmin capable user, which effectively makes it unusable.
> >
> Again, appropriate flags in inotify_init1() could fix this -- e.g., only
> fill the field (and give an error if no perms) if a flag is set.

Um, hmm... Permission is _always_ denied for 'alien' IO, as it was
pointed by Robert, at init time there is no way to know, will there be
alien IO (i.e. originated by the process with different uid) or not.
More on this: inotify initialization is just a memory allocation in
the kernel, nothing more.

We can argue about object insertion into inotify queue though. But
again, we check already that it has read permissions, and if so, we are
allowed to receive notificatons about IO against given target, since if
new code will return for whatever reason -EPERM, people will use old
code.

So, putting PID/whatever else into event can be flag-driven, but there
is no way to return EPERM anywhere in the call chain not breaking
backward compatibility of the whole idea.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[John McCutchan]

On Wed, Nov 19, 2008 at 6:53 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> Hi Michael.
>
> On Wed, Nov 19, 2008 at 09:34:46AM -0500, Michael Kerrisk (mtk.manpages@googlemail.com) wrote:
>> > So effectively you propose to have second generation of the inotify
>> > which will have additional pid field, which will be unused by all but
>> > the same uid events?
>>
>> I susepect that Christoph wants the same thing as I do: some thinking
>> towards a future-proof design, rather than a quick hack to address the needs
>> of a single application.
>
> So far the only real need is a pid. That will solve the cases I'm
> working on and it may be interesting for other applications. It is
> possible to extend read/write IO with offset and size parameters though.
>
> Do you see any other possible extensions?
>
>> > If you want to return -EPERM, than it will be _always_ returned for non
>> > sysadmin capable user, which effectively makes it unusable.
>> >
>> Again, appropriate flags in inotify_init1() could fix this -- e.g., only
>> fill the field (and give an error if no perms) if a flag is set.
>
> Um, hmm... Permission is _always_ denied for 'alien' IO, as it was
> pointed by Robert, at init time there is no way to know, will there be
> alien IO (i.e. originated by the process with different uid) or not.
> More on this: inotify initialization is just a memory allocation in
> the kernel, nothing more.
>
> We can argue about object insertion into inotify queue though. But
> again, we check already that it has read permissions, and if so, we are
> allowed to receive notificatons about IO against given target, since if
> new code will return for whatever reason -EPERM, people will use old
> code.
>
> So, putting PID/whatever else into event can be flag-driven, but there
> is no way to return EPERM anywhere in the call chain not breaking
> backward compatibility of the whole idea.


I really don't like the idea of overloading the cookie field to store
the pid for only the events that don't already use the cookie field.

Coming into this late, maybe I missed it but can you explain why you
need the pid that caused the event?

-- 
John McCutchan <john@johnmccutchan.com>

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi John.

On Thu, Nov 20, 2008 at 02:34:11PM -0800, John McCutchan (john@johnmccutchan.com) wrote:
> I really don't like the idea of overloading the cookie field to store
> the pid for only the events that don't already use the cookie field.

Adding PID or whatever else will not be used most of the time either.
I agree, that having new protocol (which so far did not get any
extensions except what I described for my own application) looks
cleaner, but if no one will use it, and existing extension works for
everyone (nothing breaks), I'm trying to push this idea up.

So, except theoretical clearness of the unused-by-everyong idea,
what forces you to think, that new inotify should be implemented?

> Coming into this late, maybe I missed it but can you explain why you
> need the pid that caused the event?

I have a network server, which gets IO requests from different clients
and maintains coherency of the data between them, but if file is
modified locally I want to flush or invalidate remote data. I decided
not to dig into the kernel on the server node and use inotify to get
notifications about events, but there is no way to determine if given IO
was originated by server itself (and in this case nothing should be
done), or by external application which accesses exported directory (and
in this case I should send update messages to clients).

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[Arnd Bergmann]

On Friday 21 November 2008, Evgeniy Polyakov wrote:
> I have a network server, which gets IO requests from different clients
> and maintains coherency of the data between them, but if file is
> modified locally I want to flush or invalidate remote data. I decided
> not to dig into the kernel on the server node and use inotify to get
> notifications about events, but there is no way to determine if given IO
> was originated by server itself (and in this case nothing should be
> done), or by external application which accesses exported directory (and
> in this case I should send update messages to clients).

The how about an inotify_init1 flag telling the kernel to ignore
changes done by the current PID? That sounds like it is potentially
useful to other applications that want to monitor the whole file system
and also write to it. It also doesn't need to change the ABI in
incompatible ways or introduce a security relevant side channel.

	Arnd <><

Re: [take 3] Use pid in inotify events.

[David Newall]

On Friday 21 November 2008, Evgeniy Polyakov wrote:
> I have a network server, which gets IO requests from different clients
> and maintains coherency of the data between them, but if file is
> modified locally I want to flush or invalidate remote data. I decided
> not to dig into the kernel on the server node and use inotify to get
> notifications about events, but there is no way to determine if given IO
> was originated by server itself (and in this case nothing should be
> done), or by external application which accesses exported directory (and
> in this case I should send update messages to clients).

Why not require local access to use the same mechanism as remote, i.e.
by "network mounting" the data on the local machine, too.  That way
there's no confusion over where the change originated nor who's copy
must be invalidated.

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Sat, Nov 22, 2008 at 05:42:21PM +1030, David Newall (davidn@davidnewall.com) wrote:
> Why not require local access to use the same mechanism as remote, i.e.
> by "network mounting" the data on the local machine, too.  That way
> there's no confusion over where the change originated nor who's copy
> must be invalidated.

There is always a possibility that some application will access given
data directly and not via mounted partition, plus I have to patch
server's kernel with out of the tree modules, so intestead I could
implement lsm-based module to catch access and embed cache coherency
protocol there. Inotify in this case is the simplest approach.

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[David Newall]

Evgeniy Polyakov wrote:
> On Sat, Nov 22, 2008 at 05:42:21PM +1030, David Newall (davidn@davidnewall.com) wrote:
>   
>> Why not require local access to use the same mechanism as remote, i.e.
>> by "network mounting" the data on the local machine, too.  That way
>> there's no confusion over where the change originated nor who's copy
>> must be invalidated.
>>     
>
> There is always a possibility that some application will access given
> data directly and not via mounted partition

Yes, there will always be ways for motivated users to trip themselves
up.  But that doesn't matter.  You can protect the user, somewhat, using
file permissions on the (outermost) directory containing your files.  If
users break through that and corrupt their data, let them, and let them
learn a lesson.  Don't try to make a foolproof system because: a) it's
likely to be a lot of work for little to no benefit, if indeed it's even
possible; and b) "make a system that even a fool can use and only a fool
will want to," as the aphorism goes.


> plus I have to patch server's kernel with out of the tree modules

Yes, that's the client in a client/server architecture.  Your server is
also a client so it's unremarkable that it would need the client software.

Don't make more work for yourself than necessary.

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

On Fri, Nov 21, 2008 at 07:39:45PM +0100, Arnd Bergmann (arnd@arndb.de) wrote:
> The how about an inotify_init1 flag telling the kernel to ignore
> changes done by the current PID? That sounds like it is potentially
> useful to other applications that want to monitor the whole file system
> and also write to it. It also doesn't need to change the ABI in
> incompatible ways or introduce a security relevant side channel.

That's a good idea. Robert, John, Michael - comments?

-- 
	Evgeniy Polyakov

Re: [take 3] Use pid in inotify events.

[John McCutchan]

At this point I don't really want to see changes made to inotify. But,
for arguments sake, why not something like inotify_init1 that takes a
flag EXTENDED_EVENT which causes a larger event structure to be used.
Something like,

struct inotify_event_extended
{
  s32 wd;
  u32 mask;
  u32 cookie;
  u32 data[4];
  char path[0];
}

The data array could be used to store arbitrary extra information,
specified by flags.

On Sat, Nov 22, 2008 at 1:37 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> On Fri, Nov 21, 2008 at 07:39:45PM +0100, Arnd Bergmann (arnd@arndb.de) wrote:
>> The how about an inotify_init1 flag telling the kernel to ignore
>> changes done by the current PID? That sounds like it is potentially
>> useful to other applications that want to monitor the whole file system
>> and also write to it. It also doesn't need to change the ABI in
>> incompatible ways or introduce a security relevant side channel.
>
> That's a good idea. Robert, John, Michael - comments?
>
> --
>        Evgeniy Polyakov
>



-- 
John McCutchan <john@johnmccutchan.com>

Re: [take 3] Use pid in inotify events.

[Evgeniy Polyakov]

Hi John.

On Sun, Nov 23, 2008 at 09:08:05PM -0800, John McCutchan (john@johnmccutchan.com) wrote:
> At this point I don't really want to see changes made to inotify. But,
> for arguments sake, why not something like inotify_init1 that takes a
> flag EXTENDED_EVENT which causes a larger event structure to be used.
> Something like,
> 
> struct inotify_event_extended
> {
>   s32 wd;
>   u32 mask;
>   u32 cookie;
>   u32 data[4];
>   char path[0];
> }
> 
> The data array could be used to store arbitrary extra information,
> specified by flags.

What will happen when above array is not enough to store needed info?
Although I do not see any reason to send start/offset for the IO itself,
but if it will be decided to do so, above array already is not large
enough. I think I will cook up preliminary patch to add nested
attributes into event structure like I described previously in the
thread to get people involved with working example.

-- 
	Evgeniy Polyakov