Fanotify API - Tracking File Movement

Fanotify API - Tracking File Movement

[Matthew Bobrowski]

Hey Jan,

I was having a brief chat with Amir the other day about an idea/use
case that I have which at present don't believe is robustly supported
by the fanotify API. I was wondering whether you could share some
thoughts on supporting the following idea.

I have a need to track file movement across a filesystem without
necessarily burdening the system by having to watch the entire
filesystem for such movements. That is, knowing when file /dir1/a had
been moved from /dir1/a to /dir2/a and then from /dir2/a to /dir3/a
and so on. Or more simply, knowing the destination/new path of the
file once it has moved.

Initially, I was thinking of using FAN_MOVE_SELF, but it doesn't quite
cut it. For such events, you only know the target location or path of
a file had been modified once it has subsequently been moved
elsewhere. Not to mention that path resolution using the file
identifier from such an event may not always work. Then there's
FAN_RENAME which could arguably work. This would include setting up a
watch on the parent directory of the file of interest and then using
the information record of type FAN_EVENT_INFO_TYPE_NEW_DFID_NAME to
figure out the new target location of the file once it has been moved
and then resetting the mark on the next parent directory once the new
target location is known. But, as Amir rightfully mentioned, this
rinse and repeat mark approach is suboptimal as it can lead to certain
race conditions.

Having briefly mentioned all this, what is your stance on maybe
extending out FAN_RENAME to also cover files? Or, maybe you have
another approach/idea in mind to cover such cases i.e. introducing a
new flag FAN_{TRACK,TRACE}.

Eager to hear your thoughts on this.

/M

Re: Fanotify API - Tracking File Movement

[Jan Kara]

Hello Matthew!

On Thu 05-05-22 20:25:31, Matthew Bobrowski wrote:
> I was having a brief chat with Amir the other day about an idea/use
> case that I have which at present don't believe is robustly supported
> by the fanotify API. I was wondering whether you could share some
> thoughts on supporting the following idea.
> 
> I have a need to track file movement across a filesystem without
> necessarily burdening the system by having to watch the entire
> filesystem for such movements. That is, knowing when file /dir1/a had
> been moved from /dir1/a to /dir2/a and then from /dir2/a to /dir3/a
> and so on. Or more simply, knowing the destination/new path of the
> file once it has moved.

OK, and the places the file moves to can be arbitrary? That seems like a
bit narrow usecase :)

> Initially, I was thinking of using FAN_MOVE_SELF, but it doesn't quite
> cut it. For such events, you only know the target location or path of
> a file had been modified once it has subsequently been moved
> elsewhere. Not to mention that path resolution using the file
> identifier from such an event may not always work. Then there's
> FAN_RENAME which could arguably work. This would include setting up a
> watch on the parent directory of the file of interest and then using
> the information record of type FAN_EVENT_INFO_TYPE_NEW_DFID_NAME to
> figure out the new target location of the file once it has been moved
> and then resetting the mark on the next parent directory once the new
> target location is known. But, as Amir rightfully mentioned, this
> rinse and repeat mark approach is suboptimal as it can lead to certain
> race conditions.

It seems to me you really want FAN_MOVE_SELF but you'd need more
information coming with it like the new parent dir, wouldn't you? It would
be relatively easy to add that info but it would kind of suck that it would
be difficult to discover in advance whether the directory info will arrive
with the event or not. But that actually would seem to be the case for
FAN_RENAME as well because we didn't seem to bother to refuse FAN_RENAME on
a file. Amir?

> Having briefly mentioned all this, what is your stance on maybe
> extending out FAN_RENAME to also cover files? Or, maybe you have
> another approach/idea in mind to cover such cases i.e. introducing a
> new flag FAN_{TRACK,TRACE}.

So extending FAN_MOVE_SELF or FAN_RENAME looks OK to me, not much thoughts
beyond that :).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

+fsdevel

On Thu, May 5, 2022 at 2:22 PM Jan Kara <jack@suse.cz> wrote:
>
> Hello Matthew!
>
> On Thu 05-05-22 20:25:31, Matthew Bobrowski wrote:
> > I was having a brief chat with Amir the other day about an idea/use
> > case that I have which at present don't believe is robustly supported
> > by the fanotify API. I was wondering whether you could share some
> > thoughts on supporting the following idea.
> >
> > I have a need to track file movement across a filesystem without
> > necessarily burdening the system by having to watch the entire
> > filesystem for such movements. That is, knowing when file /dir1/a had
> > been moved from /dir1/a to /dir2/a and then from /dir2/a to /dir3/a
> > and so on. Or more simply, knowing the destination/new path of the
> > file once it has moved.
>
> OK, and the places the file moves to can be arbitrary? That seems like a
> bit narrow usecase :)
>
> > Initially, I was thinking of using FAN_MOVE_SELF, but it doesn't quite
> > cut it. For such events, you only know the target location or path of
> > a file had been modified once it has subsequently been moved
> > elsewhere. Not to mention that path resolution using the file
> > identifier from such an event may not always work. Then there's
> > FAN_RENAME which could arguably work. This would include setting up a
> > watch on the parent directory of the file of interest and then using
> > the information record of type FAN_EVENT_INFO_TYPE_NEW_DFID_NAME to
> > figure out the new target location of the file once it has been moved
> > and then resetting the mark on the next parent directory once the new
> > target location is known. But, as Amir rightfully mentioned, this
> > rinse and repeat mark approach is suboptimal as it can lead to certain
> > race conditions.
>
> It seems to me you really want FAN_MOVE_SELF but you'd need more
> information coming with it like the new parent dir, wouldn't you? It would
> be relatively easy to add that info but it would kind of suck that it would
> be difficult to discover in advance whether the directory info will arrive
> with the event or not. But that actually would seem to be the case for
> FAN_RENAME as well because we didn't seem to bother to refuse FAN_RENAME on
> a file. Amir?
>

No, we did not, but it is also not refused for all the other dirent events and
it was never refused by inotify too, so that behavior is at least consistent.
But if we do want to change the behavior of FAN_RENAME on file, my preference
would be to start with a Fixes commit that forbis that, backport it to stable
and then allow the new behavior upstream.
I can post the fix patch.

> > Having briefly mentioned all this, what is your stance on maybe
> > extending out FAN_RENAME to also cover files? Or, maybe you have
> > another approach/idea in mind to cover such cases i.e. introducing a
> > new flag FAN_{TRACK,TRACE}.
>
> So extending FAN_MOVE_SELF or FAN_RENAME looks OK to me, not much thoughts
> beyond that :).

Both FAN_RENAME and FAN_REPORT_TARGET_FID are from v5.17
which is rather new and it is highly unlikely that anyone has ever used them,
so I think we can get away with fixing the API either way.
Not to mention that the man pages have not been updated.

This is from the man page that is pending review:

       FAN_REPORT_TARGET_FID (since Linux 5.17)
              Events for fanotify groups initialized with this flag
will contain additional information
              about the child correlated with directory entry
modification events...
              For the directory entry modification events
              FAN_CREATE,  FAN_DELETE,  FAN_RENAME,  and  FAN_MOVE,
an  additional...

       FAN_MOVED_TO (since Linux 5.1)
              Create an event when a file or directory has been moved
to a marked parent directory...

       FAN_RENAME (since Linux 5.17)
              This  event contains the same information provided by
events FAN_MOVED_FROM
              and FAN_MOVED_TO, ...

       FAN_MOVE_SELF (since Linux 5.1)
              Create an event when a marked file or directory itself
has been moved...

I think it will be easier to retrofit this functionality of FAN_RENAME
(i.e. ...provided
by events FAN_MOVED_FROM, FAN_MOVED_TO, and FAN_MOVE_SELF).
Looking at the code, I think it will also be much easier to implement
for FAN_RENAME
because it is special-cased for reporting.

HOWEVER! look at the way we implemented reporting of FAN_RENAME
(i.e. match_mask). We report_new location only if watching sb or watching
new dir. We did that for a reason because watcher may not have permissions
to read new dir. We could revisit this decision for a privileged group, but will
need to go back reading all the discussions we had about this point to see
if there were other reasons(?).

Thanks,
Amir.

Re: Fanotify API - Tracking File Movement

[Jan Kara]

On Thu 05-05-22 15:56:16, Amir Goldstein wrote:
> On Thu, May 5, 2022 at 2:22 PM Jan Kara <jack@suse.cz> wrote:
> >
> > Hello Matthew!
> >
> > On Thu 05-05-22 20:25:31, Matthew Bobrowski wrote:
> > > I was having a brief chat with Amir the other day about an idea/use
> > > case that I have which at present don't believe is robustly supported
> > > by the fanotify API. I was wondering whether you could share some
> > > thoughts on supporting the following idea.
> > >
> > > I have a need to track file movement across a filesystem without
> > > necessarily burdening the system by having to watch the entire
> > > filesystem for such movements. That is, knowing when file /dir1/a had
> > > been moved from /dir1/a to /dir2/a and then from /dir2/a to /dir3/a
> > > and so on. Or more simply, knowing the destination/new path of the
> > > file once it has moved.
> >
> > OK, and the places the file moves to can be arbitrary? That seems like a
> > bit narrow usecase :)
> >
> > > Initially, I was thinking of using FAN_MOVE_SELF, but it doesn't quite
> > > cut it. For such events, you only know the target location or path of
> > > a file had been modified once it has subsequently been moved
> > > elsewhere. Not to mention that path resolution using the file
> > > identifier from such an event may not always work. Then there's
> > > FAN_RENAME which could arguably work. This would include setting up a
> > > watch on the parent directory of the file of interest and then using
> > > the information record of type FAN_EVENT_INFO_TYPE_NEW_DFID_NAME to
> > > figure out the new target location of the file once it has been moved
> > > and then resetting the mark on the next parent directory once the new
> > > target location is known. But, as Amir rightfully mentioned, this
> > > rinse and repeat mark approach is suboptimal as it can lead to certain
> > > race conditions.
> >
> > It seems to me you really want FAN_MOVE_SELF but you'd need more
> > information coming with it like the new parent dir, wouldn't you? It would
> > be relatively easy to add that info but it would kind of suck that it would
> > be difficult to discover in advance whether the directory info will arrive
> > with the event or not. But that actually would seem to be the case for
> > FAN_RENAME as well because we didn't seem to bother to refuse FAN_RENAME on
> > a file. Amir?
> >
> 
> No, we did not, but it is also not refused for all the other dirent events and
> it was never refused by inotify too, so that behavior is at least consistent.
> But if we do want to change the behavior of FAN_RENAME on file, my preference
> would be to start with a Fixes commit that forbis that, backport it to stable
> and then allow the new behavior upstream.
> I can post the fix patch.

Yeah, I think we should do that. Thanks for looking into this!

> > > Having briefly mentioned all this, what is your stance on maybe
> > > extending out FAN_RENAME to also cover files? Or, maybe you have
> > > another approach/idea in mind to cover such cases i.e. introducing a
> > > new flag FAN_{TRACK,TRACE}.
> >
> > So extending FAN_MOVE_SELF or FAN_RENAME looks OK to me, not much thoughts
> > beyond that :).
> 
> Both FAN_RENAME and FAN_REPORT_TARGET_FID are from v5.17
> which is rather new and it is highly unlikely that anyone has ever used them,
> so I think we can get away with fixing the API either way.
> Not to mention that the man pages have not been updated.
> 
> This is from the man page that is pending review:
> 
>        FAN_REPORT_TARGET_FID (since Linux 5.17)
>               Events for fanotify groups initialized with this flag
> will contain additional information
>               about the child correlated with directory entry
> modification events...
>               For the directory entry modification events
>               FAN_CREATE,  FAN_DELETE,  FAN_RENAME,  and  FAN_MOVE,
> an  additional...
> 
>        FAN_MOVED_TO (since Linux 5.1)
>               Create an event when a file or directory has been moved
> to a marked parent directory...
> 
>        FAN_RENAME (since Linux 5.17)
>               This  event contains the same information provided by
> events FAN_MOVED_FROM
>               and FAN_MOVED_TO, ...
> 
>        FAN_MOVE_SELF (since Linux 5.1)
>               Create an event when a marked file or directory itself
> has been moved...
> 
> I think it will be easier to retrofit this functionality of FAN_RENAME
> (i.e. ...provided
> by events FAN_MOVED_FROM, FAN_MOVED_TO, and FAN_MOVE_SELF).
> Looking at the code, I think it will also be much easier to implement
> for FAN_RENAME
> because it is special-cased for reporting.
> 
> HOWEVER! look at the way we implemented reporting of FAN_RENAME
> (i.e. match_mask). We report_new location only if watching sb or watching
> new dir. We did that for a reason because watcher may not have permissions
> to read new dir. We could revisit this decision for a privileged group, but will
> need to go back reading all the discussions we had about this point to see
> if there were other reasons(?).

Yeah, this is a good point. We are able to safely report the new parent
only if the watching process is able to prove it is able to watch it.
Adding even more special cases there would be ugly and error prone I'm
afraid. We could certainly make this available only to priviledged
notification groups but still it is one more odd corner case and the
usecase does not seem to be that big. So maybe watching on sb for
FAN_RENAME and just quickly filtering based on child FID would be better
solution than fiddling with new event for files?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Fanotify API - Tracking File Movement

[Matthew Bobrowski]

On Thu, May 05, 2022 at 03:30:57PM +0200, Jan Kara wrote:
> On Thu 05-05-22 15:56:16, Amir Goldstein wrote:
> > On Thu, May 5, 2022 at 2:22 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Hello Matthew!
> > >
> > > On Thu 05-05-22 20:25:31, Matthew Bobrowski wrote:
> > > > I was having a brief chat with Amir the other day about an idea/use
> > > > case that I have which at present don't believe is robustly supported
> > > > by the fanotify API. I was wondering whether you could share some
> > > > thoughts on supporting the following idea.
> > > >
> > > > I have a need to track file movement across a filesystem without
> > > > necessarily burdening the system by having to watch the entire
> > > > filesystem for such movements. That is, knowing when file /dir1/a had
> > > > been moved from /dir1/a to /dir2/a and then from /dir2/a to /dir3/a
> > > > and so on. Or more simply, knowing the destination/new path of the
> > > > file once it has moved.
> > >
> > > OK, and the places the file moves to can be arbitrary? That seems like a
> > > bit narrow usecase :)
> > >
> > > > Initially, I was thinking of using FAN_MOVE_SELF, but it doesn't quite
> > > > cut it. For such events, you only know the target location or path of
> > > > a file had been modified once it has subsequently been moved
> > > > elsewhere. Not to mention that path resolution using the file
> > > > identifier from such an event may not always work. Then there's
> > > > FAN_RENAME which could arguably work. This would include setting up a
> > > > watch on the parent directory of the file of interest and then using
> > > > the information record of type FAN_EVENT_INFO_TYPE_NEW_DFID_NAME to
> > > > figure out the new target location of the file once it has been moved
> > > > and then resetting the mark on the next parent directory once the new
> > > > target location is known. But, as Amir rightfully mentioned, this
> > > > rinse and repeat mark approach is suboptimal as it can lead to certain
> > > > race conditions.
> > >
> > > It seems to me you really want FAN_MOVE_SELF but you'd need more
> > > information coming with it like the new parent dir, wouldn't you? It would
> > > be relatively easy to add that info but it would kind of suck that it would
> > > be difficult to discover in advance whether the directory info will arrive
> > > with the event or not. But that actually would seem to be the case for
> > > FAN_RENAME as well because we didn't seem to bother to refuse FAN_RENAME on
> > > a file. Amir?
> > >
> > 
> > No, we did not, but it is also not refused for all the other dirent events and
> > it was never refused by inotify too, so that behavior is at least consistent.
> > But if we do want to change the behavior of FAN_RENAME on file, my preference
> > would be to start with a Fixes commit that forbis that, backport it to stable
> > and then allow the new behavior upstream.
> > I can post the fix patch.
> 
> Yeah, I think we should do that. Thanks for looking into this!
> 
> > > > Having briefly mentioned all this, what is your stance on maybe
> > > > extending out FAN_RENAME to also cover files? Or, maybe you have
> > > > another approach/idea in mind to cover such cases i.e. introducing a
> > > > new flag FAN_{TRACK,TRACE}.
> > >
> > > So extending FAN_MOVE_SELF or FAN_RENAME looks OK to me, not much thoughts
> > > beyond that :).
> > 
> > Both FAN_RENAME and FAN_REPORT_TARGET_FID are from v5.17
> > which is rather new and it is highly unlikely that anyone has ever used them,
> > so I think we can get away with fixing the API either way.
> > Not to mention that the man pages have not been updated.
> > 
> > This is from the man page that is pending review:
> > 
> >        FAN_REPORT_TARGET_FID (since Linux 5.17)
> >               Events for fanotify groups initialized with this flag
> > will contain additional information
> >               about the child correlated with directory entry
> > modification events...
> >               For the directory entry modification events
> >               FAN_CREATE,  FAN_DELETE,  FAN_RENAME,  and  FAN_MOVE,
> > an  additional...
> > 
> >        FAN_MOVED_TO (since Linux 5.1)
> >               Create an event when a file or directory has been moved
> > to a marked parent directory...
> > 
> >        FAN_RENAME (since Linux 5.17)
> >               This  event contains the same information provided by
> > events FAN_MOVED_FROM
> >               and FAN_MOVED_TO, ...
> > 
> >        FAN_MOVE_SELF (since Linux 5.1)
> >               Create an event when a marked file or directory itself
> > has been moved...
> > 
> > I think it will be easier to retrofit this functionality of FAN_RENAME
> > (i.e. ...provided
> > by events FAN_MOVED_FROM, FAN_MOVED_TO, and FAN_MOVE_SELF).
> > Looking at the code, I think it will also be much easier to implement
> > for FAN_RENAME
> > because it is special-cased for reporting.
> > 
> > HOWEVER! look at the way we implemented reporting of FAN_RENAME
> > (i.e. match_mask). We report_new location only if watching sb or watching
> > new dir. We did that for a reason because watcher may not have permissions
> > to read new dir. We could revisit this decision for a privileged group, but will
> > need to go back reading all the discussions we had about this point to see
> > if there were other reasons(?).
> 
> Yeah, this is a good point. We are able to safely report the new parent
> only if the watching process is able to prove it is able to watch it.
> Adding even more special cases there would be ugly and error prone I'm
> afraid. We could certainly make this available only to priviledged
> notification groups but still it is one more odd corner case and the
> usecase does not seem to be that big.

Sorry, I'm confused about the conclusion we've drawn here. Are we hard
up against not extending FAN_RENAME for the sole reason that the
implementation might be ugly and error prone?

Can we not expose this case exclusively to privileged notification
groups/watchers? This case seems far simpler than what has already
been implemented in the FAN_RENAME series, that is as you mentioned,
trying to safely report the new parent only if the watching process is
able to prove it is able to watch it. If anything, I would've expected
the privileged case to be implemented prior to attempting to cover
whether the super block or target directory is being watched.

> So maybe watching on sb for FAN_RENAME and just quickly filtering
> based on child FID would be better solution than fiddling with new
> event for files?

Ah, I really wanted to stay away from watching the super block for all
FAN_RENAME events. I feel like userspace wearing the pain for such
cases is suboptimal, as this is something that can effectively be done
in-kernel.

/M

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

> > > HOWEVER! look at the way we implemented reporting of FAN_RENAME
> > > (i.e. match_mask). We report_new location only if watching sb or watching
> > > new dir. We did that for a reason because watcher may not have permissions
> > > to read new dir. We could revisit this decision for a privileged group, but will
> > > need to go back reading all the discussions we had about this point to see
> > > if there were other reasons(?).
> >
> > Yeah, this is a good point. We are able to safely report the new parent
> > only if the watching process is able to prove it is able to watch it.
> > Adding even more special cases there would be ugly and error prone I'm
> > afraid. We could certainly make this available only to priviledged
> > notification groups but still it is one more odd corner case and the
> > usecase does not seem to be that big.
>
> Sorry, I'm confused about the conclusion we've drawn here. Are we hard
> up against not extending FAN_RENAME for the sole reason that the
> implementation might be ugly and error prone?
>
> Can we not expose this case exclusively to privileged notification
> groups/watchers? This case seems far simpler than what has already
> been implemented in the FAN_RENAME series, that is as you mentioned,
> trying to safely report the new parent only if the watching process is
> able to prove it is able to watch it. If anything, I would've expected
> the privileged case to be implemented prior to attempting to cover
> whether the super block or target directory is being watched.

To be fair, that is what the "added complexity" for the privileged use
case looks like:

                        /* Report both old and new parent+name if sb watching */
                        report_old = report_new =
+                               !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) ||
                                match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
                        report_old |=
                                match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);

There is a bit more complexity to replace FSNOTIFY_ITER_TYPE_INODE2
with FSNOTIFY_ITER_TYPE_DIR1 and FSNOTIFY_ITER_TYPE_DIR1.

But I understand why Jan is hesitant about increasing the cases for
already highly
specialized code.

My only argument in favor of this case is that had we though about it before
merging FAN_RENAME we would have probably included it.(?)

>
> > So maybe watching on sb for FAN_RENAME and just quickly filtering
> > based on child FID would be better solution than fiddling with new
> > event for files?
>
> Ah, I really wanted to stay away from watching the super block for all
> FAN_RENAME events. I feel like userspace wearing the pain for such
> cases is suboptimal, as this is something that can effectively be done
> in-kernel.
>

I'm not opposing. Letting Jan make the call.

Thanks,
Amir.

Re: Fanotify API - Tracking File Movement

[Jan Kara]

On Fri 06-05-22 03:00:58, Amir Goldstein wrote:
> > > > HOWEVER! look at the way we implemented reporting of FAN_RENAME
> > > > (i.e. match_mask). We report_new location only if watching sb or watching
> > > > new dir. We did that for a reason because watcher may not have permissions
> > > > to read new dir. We could revisit this decision for a privileged group, but will
> > > > need to go back reading all the discussions we had about this point to see
> > > > if there were other reasons(?).
> > >
> > > Yeah, this is a good point. We are able to safely report the new parent
> > > only if the watching process is able to prove it is able to watch it.
> > > Adding even more special cases there would be ugly and error prone I'm
> > > afraid. We could certainly make this available only to priviledged
> > > notification groups but still it is one more odd corner case and the
> > > usecase does not seem to be that big.
> >
> > Sorry, I'm confused about the conclusion we've drawn here. Are we hard
> > up against not extending FAN_RENAME for the sole reason that the
> > implementation might be ugly and error prone?
> >
> > Can we not expose this case exclusively to privileged notification
> > groups/watchers? This case seems far simpler than what has already
> > been implemented in the FAN_RENAME series, that is as you mentioned,
> > trying to safely report the new parent only if the watching process is
> > able to prove it is able to watch it. If anything, I would've expected
> > the privileged case to be implemented prior to attempting to cover
> > whether the super block or target directory is being watched.
> 
> To be fair, that is what the "added complexity" for the privileged use
> case looks like:
> 
>                         /* Report both old and new parent+name if sb watching */
>                         report_old = report_new =
> +                               !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) ||
>                                 match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
>                         report_old |=
>                                 match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);
> 
> There is a bit more complexity to replace FSNOTIFY_ITER_TYPE_INODE2
> with FSNOTIFY_ITER_TYPE_DIR1 and FSNOTIFY_ITER_TYPE_DIR1.
> 
> But I understand why Jan is hesitant about increasing the cases for
> already highly
> specialized code.
> 
> My only argument in favor of this case is that had we though about it before
> merging FAN_RENAME we would have probably included it.(?)

So I've slept on it and agree that allowing FAN_RENAME on a file with the
semantics Matthew wants is consistent with the current design and probably
the only sensible meaning we can give to it. I also agree that updating
permission checks for reporting parent dirs isn't that big of a headache
and maintenance burden going further.

I'm still somewhat concerned about how the propagation of two parent
directories and then formatting into the event is going to work out (i.e.,
how many special cases that's going to need) but I'm willing to have a look
at the patch. Maybe it won't be as bad as I was afraid :).

> > Ah, I really wanted to stay away from watching the super block for all
> > FAN_RENAME events. I feel like userspace wearing the pain for such
> > cases is suboptimal, as this is something that can effectively be done
> > in-kernel.

I agree that kernel can do this more efficiently than userspace but the
question is how much in terms of code (and thus maintenance overhead) are
we willing to spend for this IMO rather specialized feature. The code to
build necessary information to pass with the event, dealing with all
different types of watches and backends and then formatting it to the event
for userspace is complex as hell. Whenever I have to do or review some
non-trivial changes to it, my head hurts ;) And the amount of subtle
cornercase bugs we were fixing in that code over the years is just a
testimony of this. So that's why I'm reluctant to add even small
complications to it for something I find relatively specialized use (think
for how many userspace programs this feature is going to be useful, I don't
think many).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

> So I've slept on it and agree that allowing FAN_RENAME on a file with the
> semantics Matthew wants is consistent with the current design and probably
> the only sensible meaning we can give to it. I also agree that updating
> permission checks for reporting parent dirs isn't that big of a headache
> and maintenance burden going further.
>
> I'm still somewhat concerned about how the propagation of two parent
> directories and then formatting into the event is going to work out (i.e.,
> how many special cases that's going to need) but I'm willing to have a look
> at the patch. Maybe it won't be as bad as I was afraid :).

Here is a patch. I don't think it is so bad.(?)
In fact, I think the code is made a bit more clear when we stop overloading
ITER_TYPE_INODE as the new_dir in FS_RENAME case.
But yes, it does add yet another special (moved non-dir vs. moved dir).

Sorry Matthew, I was looking at the code to give you pointers, but there were
so many subtle details (as Jan has expected) that I could only communicate
them with a patch.
I tested that this patch does not break anything, but did not implement the
UAPI changes, so the functionality that it adds is not tested - I leave that
to you.

>
> > > Ah, I really wanted to stay away from watching the super block for all
> > > FAN_RENAME events. I feel like userspace wearing the pain for such
> > > cases is suboptimal, as this is something that can effectively be done
> > > in-kernel.
>
> I agree that kernel can do this more efficiently than userspace but the
> question is how much in terms of code (and thus maintenance overhead) are
> we willing to spend for this IMO rather specialized feature. The code to
> build necessary information to pass with the event, dealing with all
> different types of watches and backends and then formatting it to the event
> for userspace is complex as hell. Whenever I have to do or review some
> non-trivial changes to it, my head hurts ;) And the amount of subtle
> cornercase bugs we were fixing in that code over the years is just a
> testimony of this. So that's why I'm reluctant to add even small
> complications to it for something I find relatively specialized use (think
> for how many userspace programs this feature is going to be useful, I don't
> think many).
>

My 0.02$ - while FAN_RENAME is a snowflake, this is not because
of our design, this is because rename(2) is a snowflake vfs operation.
The event information simply reflects the operation complexity and when
looking at non-Linux filesystem event APIs, the event information for rename
looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
was enhanced at some point exactly as we did with FAN_RENAME to
have all the info in one event vs. having to join two events.

Hopefully, the attached patch simplifies the specialized implementation
a little bit.

But... (there is always a but when it comes to UAPI),
When looking at my patch, one cannot help wondering -
what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
If those can report child fid, why should they be treated differently
than FAN_RENAME w.r.t marking the child inode?

For example, when watching a non-dir for FAN_CREATE, it could
be VERY helpful to get the dirfid+name of where the inode was
hard linked. In fact, if an application is watching FAN_RENAME to track
the movement of a non-dir file and does not watch hardlink+unlink, then
the file could escape under the application's nose.

We should definitely not extend the UAPI to a non-dir file before we provide
an answer to this question. For that reason I also sent v2 of Fix patch to
deny setting all dirent events on non-dir with FAN_REPORT_TARGET_FID.

Thanks,
Amir.

[-- Attachment #2: 0001-fsnotify-send-FS_RENAME-to-groups-watching-the-moved.patch --]
[-- Type: text/x-patch, Size: 9195 bytes --]

From d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Sat, 7 May 2022 10:04:08 +0300
Subject: [PATCH] fsnotify: send FS_RENAME to groups watching the moved inode

Send FS_RENAME to groups watching the moved inode itself if the
moved inode is a non-dir to allow tracking the movement of a watched
inode.

Sending FS_RENAME to a moved watched directory would be confusing
and FAN_MOVE_SELF provided enough information to track the movements
of a watched directory.

At the moment, no backend allows watching FS_RENAME on a non-dir inode.
When backends (i.e. fanotify) will allow watching FS_RENAME on a non-dir
inode, old/new dir+name should be reported only if the group proves to
have read permission on old/new dir.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    |  4 +-
 fs/notify/fsnotify.c             | 72 ++++++++++++++++++++------------
 include/linux/fsnotify.h         | 19 +++++----
 include/linux/fsnotify_backend.h |  6 ++-
 4 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 985e995d2a39..fc498fc090da 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -780,9 +780,9 @@ static struct fanotify_event *fanotify_alloc_event(
 			report_old = report_new =
 				match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
 			report_old |=
-				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);
+				match_mask & (1U << FSNOTIFY_ITER_TYPE_OLD_DIR);
 			report_new |=
-				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE2);
+				match_mask & (1U << FSNOTIFY_ITER_TYPE_NEW_DIR);
 
 			if (!report_old) {
 				/* Do not report old parent+name */
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 6eee19d15e8c..ce1ae725ec8c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -277,19 +277,19 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
 		return 0;
 
-	/*
-	 * For FS_RENAME, 'dir' is old dir and 'data' is new dentry.
-	 * The only ->handle_inode_event() backend that supports FS_RENAME is
-	 * dnotify, where it means file was renamed within same parent.
-	 */
 	if (mask & FS_RENAME) {
-		struct dentry *moved = fsnotify_data_dentry(data, data_type);
+		inode_mark = fsnotify_iter_old_dir_mark(iter_info);
+		parent_mark = fsnotify_iter_new_dir_mark(iter_info);
 
-		if (dir != moved->d_parent->d_inode)
+		/*
+		 * The only ->handle_inode_event() backend that supports
+		 * FS_RENAME is dnotify, where DN_RENAME means that file
+		 * was renamed within the same parent.
+		 */
+		if (WARN_ON_ONCE(!inode_mark) ||
+		    inode_mark != parent_mark)
 			return 0;
-	}
-
-	if (parent_mark) {
+	} else if (parent_mark) {
 		/*
 		 * parent_mark indicates that the parent inode is watching
 		 * children and interested in this event, which is an event
@@ -479,9 +479,9 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	struct super_block *sb = fsnotify_data_sb(data, data_type);
 	struct fsnotify_iter_info iter_info = {};
 	struct mount *mnt = NULL;
-	struct inode *inode2 = NULL;
+	struct inode *dir1, *dir2;
 	struct dentry *moved;
-	int inode2_type;
+	int dir1_type = 0;
 	int ret = 0;
 	__u32 test_mask, marks_mask;
 
@@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (!inode) {
 		/* Dirent event - report on TYPE_INODE to dir */
 		inode = dir;
-		/* For FS_RENAME, inode is old_dir and inode2 is new_dir */
-		if (mask & FS_RENAME) {
-			moved = fsnotify_data_dentry(data, data_type);
-			inode2 = moved->d_parent->d_inode;
-			inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
-		}
+	} else if (mask & FS_RENAME) {
+		/* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
+		moved = fsnotify_data_dentry(data, data_type);
+		dir1 = moved->d_parent->d_inode;
+		dir2 = dir;
+		if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
+			dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
+		/*
+		 * Send FS_RENAME to groups watching the moved inode itself
+		 * only if the moved inode is a non-dir.
+		 * Sending FS_RENAME to a moved watched directory would be
+		 * confusing and FS_MOVE_SELF provided enough information to
+		 * track the movements of a watched directory.
+		 */
+		if (mask & FS_ISDIR)
+			inode = NULL;
 	} else if (mask & FS_EVENT_ON_CHILD) {
 		/*
 		 * Event on child - report on TYPE_PARENT to dir if it is
 		 * watching children and on TYPE_INODE to child.
 		 */
-		inode2 = dir;
-		inode2_type = FSNOTIFY_ITER_TYPE_PARENT;
+		dir1 = dir;
+		dir2 = NULL;
+		if (dir1->i_fsnotify_marks)
+			dir1_type = FSNOTIFY_ITER_TYPE_PARENT;
 	}
 
 	/*
@@ -516,7 +528,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (!sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
 	    (!inode || !inode->i_fsnotify_marks) &&
-	    (!inode2 || !inode2->i_fsnotify_marks))
+	    !dir1_type)
 		return 0;
 
 	marks_mask = sb->s_fsnotify_mask;
@@ -524,8 +536,12 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		marks_mask |= mnt->mnt_fsnotify_mask;
 	if (inode)
 		marks_mask |= inode->i_fsnotify_mask;
-	if (inode2)
-		marks_mask |= inode2->i_fsnotify_mask;
+	if (dir1_type) {
+		if (dir1)
+			marks_mask |= dir1->i_fsnotify_mask;
+		if (dir2)
+			marks_mask |= dir2->i_fsnotify_mask;
+	}
 
 
 	/*
@@ -550,9 +566,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		iter_info.marks[FSNOTIFY_ITER_TYPE_INODE] =
 			fsnotify_first_mark(&inode->i_fsnotify_marks);
 	}
-	if (inode2) {
-		iter_info.marks[inode2_type] =
-			fsnotify_first_mark(&inode2->i_fsnotify_marks);
+	if (dir1_type) {
+		if (dir1)
+			iter_info.marks[dir1_type] =
+				fsnotify_first_mark(&dir1->i_fsnotify_marks);
+		if (dir2)
+			iter_info.marks[FSNOTIFY_ITER_TYPE_NEW_DIR] =
+				fsnotify_first_mark(&dir2->i_fsnotify_marks);
 	}
 
 	/*
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index bb8467cd11ae..75f1048443a5 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -28,18 +28,19 @@
  */
 static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
 				struct inode *dir, const struct qstr *name,
-				u32 cookie)
+				struct inode *inode, u32 cookie)
 {
 	if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
 		return 0;
 
-	return fsnotify(mask, data, data_type, dir, name, NULL, cookie);
+	return fsnotify(mask, data, data_type, dir, name, inode, cookie);
 }
 
 static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 				   __u32 mask)
 {
-	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name, 0);
+	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name,
+		      NULL, 0);
 }
 
 static inline void fsnotify_inode(struct inode *inode, __u32 mask)
@@ -154,13 +155,13 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	}
 
 	/* Event with information about both old and new parent+name */
-	fsnotify_name(rename_mask, moved, FSNOTIFY_EVENT_DENTRY,
-		      old_dir, old_name, 0);
+	fsnotify(rename_mask, moved, FSNOTIFY_EVENT_DENTRY,
+		 old_dir, old_name, source, 0);
 
 	fsnotify_name(old_dir_mask, source, FSNOTIFY_EVENT_INODE,
-		      old_dir, old_name, fs_cookie);
+		      old_dir, old_name, NULL, fs_cookie);
 	fsnotify_name(new_dir_mask, source, FSNOTIFY_EVENT_INODE,
-		      new_dir, new_name, fs_cookie);
+		      new_dir, new_name, NULL, fs_cookie);
 
 	if (target)
 		fsnotify_link_count(target);
@@ -221,7 +222,7 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode,
 	audit_inode_child(dir, new_dentry, AUDIT_TYPE_CHILD_CREATE);
 
 	fsnotify_name(FS_CREATE, inode, FSNOTIFY_EVENT_INODE,
-		      dir, &new_dentry->d_name, 0);
+		      dir, &new_dentry->d_name, NULL, 0);
 }
 
 /*
@@ -241,7 +242,7 @@ static inline void fsnotify_delete(struct inode *dir, struct inode *inode,
 		mask |= FS_ISDIR;
 
 	fsnotify_name(mask, inode, FSNOTIFY_EVENT_INODE, dir, &dentry->d_name,
-		      0);
+		      NULL, 0);
 }
 
 /**
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9a1a9e78f69f..15c47e8b4ae3 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -374,11 +374,13 @@ static inline struct fs_error_report *fsnotify_data_error_report(
  * For example, both parent and child are watching an object of type inode.
  */
 enum fsnotify_iter_type {
+	FSNOTIFY_ITER_TYPE_NONE = 0,
 	FSNOTIFY_ITER_TYPE_INODE,
 	FSNOTIFY_ITER_TYPE_VFSMOUNT,
 	FSNOTIFY_ITER_TYPE_SB,
 	FSNOTIFY_ITER_TYPE_PARENT,
-	FSNOTIFY_ITER_TYPE_INODE2,
+	FSNOTIFY_ITER_TYPE_OLD_DIR,
+	FSNOTIFY_ITER_TYPE_NEW_DIR,
 	FSNOTIFY_ITER_TYPE_COUNT
 };
 
@@ -433,6 +435,8 @@ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
 FSNOTIFY_ITER_FUNCS(parent, PARENT)
+FSNOTIFY_ITER_FUNCS(old_dir, OLD_DIR)
+FSNOTIFY_ITER_FUNCS(new_dir, NEW_DIR)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 FSNOTIFY_ITER_FUNCS(sb, SB)
 
-- 
2.25.1

Re: Fanotify API - Tracking File Movement

[Jan Kara]

On Sat 07-05-22 19:03:13, Amir Goldstein wrote:
> > So I've slept on it and agree that allowing FAN_RENAME on a file with the
> > semantics Matthew wants is consistent with the current design and probably
> > the only sensible meaning we can give to it. I also agree that updating
> > permission checks for reporting parent dirs isn't that big of a headache
> > and maintenance burden going further.
> >
> > I'm still somewhat concerned about how the propagation of two parent
> > directories and then formatting into the event is going to work out (i.e.,
> > how many special cases that's going to need) but I'm willing to have a look
> > at the patch. Maybe it won't be as bad as I was afraid :).
> 
> Here is a patch. I don't think it is so bad.(?)
> In fact, I think the code is made a bit more clear when we stop overloading
> ITER_TYPE_INODE as the new_dir in FS_RENAME case.
> But yes, it does add yet another special (moved non-dir vs. moved dir).

Yeah, it looks fine.

> > > > Ah, I really wanted to stay away from watching the super block for all
> > > > FAN_RENAME events. I feel like userspace wearing the pain for such
> > > > cases is suboptimal, as this is something that can effectively be done
> > > > in-kernel.
> >
> > I agree that kernel can do this more efficiently than userspace but the
> > question is how much in terms of code (and thus maintenance overhead) are
> > we willing to spend for this IMO rather specialized feature. The code to
> > build necessary information to pass with the event, dealing with all
> > different types of watches and backends and then formatting it to the event
> > for userspace is complex as hell. Whenever I have to do or review some
> > non-trivial changes to it, my head hurts ;) And the amount of subtle
> > cornercase bugs we were fixing in that code over the years is just a
> > testimony of this. So that's why I'm reluctant to add even small
> > complications to it for something I find relatively specialized use (think
> > for how many userspace programs this feature is going to be useful, I don't
> > think many).
> >
> 
> My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> of our design, this is because rename(2) is a snowflake vfs operation.
> The event information simply reflects the operation complexity and when
> looking at non-Linux filesystem event APIs, the event information for rename
> looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> was enhanced at some point exactly as we did with FAN_RENAME to
> have all the info in one event vs. having to join two events.

I agree that rename is a special operation and that reflects to some extent
in fsnotify as well.


> But... (there is always a but when it comes to UAPI),
> When looking at my patch, one cannot help wondering -
> what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> If those can report child fid, why should they be treated differently
> than FAN_RENAME w.r.t marking the child inode?
> 
> For example, when watching a non-dir for FAN_CREATE, it could
> be VERY helpful to get the dirfid+name of where the inode was
> hard linked. In fact, if an application is watching FAN_RENAME to track
> the movement of a non-dir file and does not watch hardlink+unlink, then
> the file could escape under the application's nose.

I agree that being able to track FAN_CREATE (or FAN_DELETE for that matter
if you'd like to maintain in which dirs a file is visible) for a file like
this would be useful in a similar way as what we now enable for rename.

> We should definitely not extend the UAPI to a non-dir file before we provide
> an answer to this question. For that reason I also sent v2 of Fix patch to
> deny setting all dirent events on non-dir with FAN_REPORT_TARGET_FID.

Agreed.

> From d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <amir73il@gmail.com>
> Date: Sat, 7 May 2022 10:04:08 +0300
> Subject: [PATCH] fsnotify: send FS_RENAME to groups watching the moved inode
> 
> Send FS_RENAME to groups watching the moved inode itself if the
> moved inode is a non-dir to allow tracking the movement of a watched
> inode.
> 
> Sending FS_RENAME to a moved watched directory would be confusing
> and FAN_MOVE_SELF provided enough information to track the movements
> of a watched directory.
> 
> At the moment, no backend allows watching FS_RENAME on a non-dir inode.
> When backends (i.e. fanotify) will allow watching FS_RENAME on a non-dir
> inode, old/new dir+name should be reported only if the group proves to
> have read permission on old/new dir.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c    |  4 +-
>  fs/notify/fsnotify.c             | 72 ++++++++++++++++++++------------
>  include/linux/fsnotify.h         | 19 +++++----
>  include/linux/fsnotify_backend.h |  6 ++-
>  4 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..fc498fc090da 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -780,9 +780,9 @@ static struct fanotify_event *fanotify_alloc_event(
>  			report_old = report_new =
>  				match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
>  			report_old |=
> -				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);
> +				match_mask & (1U << FSNOTIFY_ITER_TYPE_OLD_DIR);
>  			report_new |=
> -				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE2);
> +				match_mask & (1U << FSNOTIFY_ITER_TYPE_NEW_DIR);
>  
>  			if (!report_old) {
>  				/* Do not report old parent+name */
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 6eee19d15e8c..ce1ae725ec8c 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -277,19 +277,19 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
>  	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
>  		return 0;
>  
> -	/*
> -	 * For FS_RENAME, 'dir' is old dir and 'data' is new dentry.
> -	 * The only ->handle_inode_event() backend that supports FS_RENAME is
> -	 * dnotify, where it means file was renamed within same parent.
> -	 */
>  	if (mask & FS_RENAME) {
> -		struct dentry *moved = fsnotify_data_dentry(data, data_type);
> +		inode_mark = fsnotify_iter_old_dir_mark(iter_info);
> +		parent_mark = fsnotify_iter_new_dir_mark(iter_info);

AFAIU you use parent_mark here as a temporary variable. Maybe it would be
less confusing to just declare new 'new_dir_mark' variable to use for
comparison here? Also we would not have to change the "if (parent_mark)"
condition below.
  
> -		if (dir != moved->d_parent->d_inode)
> +		/*
> +		 * The only ->handle_inode_event() backend that supports
> +		 * FS_RENAME is dnotify, where DN_RENAME means that file
> +		 * was renamed within the same parent.
> +		 */
> +		if (WARN_ON_ONCE(!inode_mark) ||
> +		    inode_mark != parent_mark)
>  			return 0;
> -	}
> -
> -	if (parent_mark) {
> +	} else if (parent_mark) {
>  		/*
>  		 * parent_mark indicates that the parent inode is watching
>  		 * children and interested in this event, which is an event

...

> @@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  	if (!inode) {
>  		/* Dirent event - report on TYPE_INODE to dir */
>  		inode = dir;
> -		/* For FS_RENAME, inode is old_dir and inode2 is new_dir */
> -		if (mask & FS_RENAME) {
> -			moved = fsnotify_data_dentry(data, data_type);
> -			inode2 = moved->d_parent->d_inode;
> -			inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
> -		}
> +	} else if (mask & FS_RENAME) {
> +		/* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> +		moved = fsnotify_data_dentry(data, data_type);
> +		dir1 = moved->d_parent->d_inode;
> +		dir2 = dir;
> +		if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> +			dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> +		/*
> +		 * Send FS_RENAME to groups watching the moved inode itself
> +		 * only if the moved inode is a non-dir.
> +		 * Sending FS_RENAME to a moved watched directory would be
> +		 * confusing and FS_MOVE_SELF provided enough information to
> +		 * track the movements of a watched directory.
> +		 */
> +		if (mask & FS_ISDIR)
> +			inode = NULL;

So I agree that sending FS_RENAME to a directory when the directory itself
moves is confusing. But then it makes me wonder whether it would not be
more logical if we extended FS_MOVE_SELF rather than FS_RENAME. Currently
FS_MOVE_SELF is an inode event but we could expand it to provide new parent
directory to priviledged listeners (and even old directory if we wanted).
But I guess the concern is that we'd have to introduce a group flag for
this to make sure things are backward compatible, whereas with FAN_RENAME
we could mostly get away without a feature flag?

> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index bb8467cd11ae..75f1048443a5 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -28,18 +28,19 @@
>   */
>  static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
>  				struct inode *dir, const struct qstr *name,
> -				u32 cookie)
> +				struct inode *inode, u32 cookie)

So you add 'inode' argument to fsnotify_name() but never actually use it?
The only place where inode would be non-NULL is changed to use fsnotify()
directly...

Also I'm not sure why you pass the inode for rename event now when the same
inode is passed as 'dentry' in data? I feel like I'm missing something
here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

> > @@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> >       if (!inode) {
> >               /* Dirent event - report on TYPE_INODE to dir */
> >               inode = dir;
> > -             /* For FS_RENAME, inode is old_dir and inode2 is new_dir */
> > -             if (mask & FS_RENAME) {
> > -                     moved = fsnotify_data_dentry(data, data_type);
> > -                     inode2 = moved->d_parent->d_inode;
> > -                     inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
> > -             }
> > +     } else if (mask & FS_RENAME) {
> > +             /* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> > +             moved = fsnotify_data_dentry(data, data_type);
> > +             dir1 = moved->d_parent->d_inode;
> > +             dir2 = dir;
> > +             if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> > +                     dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> > +             /*
> > +              * Send FS_RENAME to groups watching the moved inode itself
> > +              * only if the moved inode is a non-dir.
> > +              * Sending FS_RENAME to a moved watched directory would be
> > +              * confusing and FS_MOVE_SELF provided enough information to
> > +              * track the movements of a watched directory.
> > +              */
> > +             if (mask & FS_ISDIR)
> > +                     inode = NULL;
>
> So I agree that sending FS_RENAME to a directory when the directory itself
> moves is confusing. But then it makes me wonder whether it would not be
> more logical if we extended FS_MOVE_SELF rather than FS_RENAME. Currently
> FS_MOVE_SELF is an inode event but we could expand it to provide new parent
> directory to priviledged listeners (and even old directory if we wanted).
> But I guess the concern is that we'd have to introduce a group flag for
> this to make sure things are backward compatible, whereas with FAN_RENAME
> we could mostly get away without a feature flag?
>

This thought has crossed my mind.
There are several issues with extending FS_MOVE_SELF besides the one
that you mentioned.
1. It is not needed for tracking movement of dir - the only reason we need to
    extend rename/create/delete for non-dir is because fid alone is not enough
    to find the moved inode new location
2. While we could extend FS_MOVE_SELF, we cannot extend FS_DELETE_SELF
    (and there is no FS_CREATE_SELF), so it is better to extend all the
    dirent events and not any self events
3. But the nock-out argument is the one that you wrote - we can (ab)use
    FAN_ERPORT_TARGET_FID as the backward compat barrier for changing
    the behavior of dirent events and it would almost look like we had
thought about
    this in advance ;)

> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index bb8467cd11ae..75f1048443a5 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -28,18 +28,19 @@
> >   */
> >  static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
> >                               struct inode *dir, const struct qstr *name,
> > -                             u32 cookie)
> > +                             struct inode *inode, u32 cookie)
>
> So you add 'inode' argument to fsnotify_name() but never actually use it?
> The only place where inode would be non-NULL is changed to use fsnotify()
> directly...

My bad, I wasn't going to extend fsnotify_name(), but then I thought of the
FAN_CREATE/FAN_DELETE case, so I wanted to make this more obvious
in the code that dirent events should be reported to the child inode,
but I forget
to use the extended fsnotify_name() for FAN_RENAME and left it using fsnotify().

>
> Also I'm not sure why you pass the inode for rename event now when the same
> inode is passed as 'dentry' in data? I feel like I'm missing something
> here.

The semantics of @inode argument vs. fsnotify_data_inode() are hard to
remember, but we documented them in fsnotify().

The meaning of @inode is that an event should be reported to @inode if inode
has a mark. Surely, one of the reasons for this distinction was fsnotify_name()
which carries the child inode information, but was not traditionally reported
to the child inode mark.

So when I pass positive @inode with FS_RENAME that is all it takes to
report the event on a watching child inode. The only needed minor change in
fsnotify() logic was to move if (mask & FS_RENAME) outside of if (!inode) {}.
All the rest is just tidying up the code.

If we are going to change fsnotify_name() then we need to see if there are
any other cases left where @inode is NULL and fsnotify_data_inode() is positive
(I didn't check) If not, then we may be able to drop the @inode argument.

Thanks,
Amir.

Re: Fanotify API - Tracking File Movement

[Matthew Bobrowski]

On Sat, May 07, 2022 at 07:03:13PM +0300, Amir Goldstein wrote:
> Sorry Matthew, I was looking at the code to give you pointers, but there were
> so many subtle details (as Jan has expected) that I could only communicate
> them with a patch.
> I tested that this patch does not break anything, but did not implement the
> UAPI changes, so the functionality that it adds is not tested - I leave that
> to you.

No, that's totally fine. I had to familiarize myself with the
FS/FAN_RENAME implementation as I hadn't gone over that series. So
appreciate you whipping this together quickly as it would've taken a
fair bit of time.

Before the UAPI related modifications, we need to first figure out how
we are to handle the CREATE/DELETE/MOVE cases.

...

> My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> of our design, this is because rename(2) is a snowflake vfs operation.
> The event information simply reflects the operation complexity and when
> looking at non-Linux filesystem event APIs, the event information for rename
> looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> was enhanced at some point exactly as we did with FAN_RENAME to
> have all the info in one event vs. having to join two events.
> 
> Hopefully, the attached patch simplifies the specialized implementation
> a little bit.
> 
> But... (there is always a but when it comes to UAPI),
> When looking at my patch, one cannot help wondering -
> what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> If those can report child fid, why should they be treated differently
> than FAN_RENAME w.r.t marking the child inode?

This is something that crossed my mind while looking over the patch
and is a very good thing to call-out indeed. I am of the opinion that
we shouldn't be placing FAN_RENAME in the special egg basket and also
consider how this is to operate for events
FAN_CREATE/FAN_DELETE/FAN_MOVE.

> For example, when watching a non-dir for FAN_CREATE, it could
> be VERY helpful to get the dirfid+name of where the inode was
> hard linked.

Oh right, here you're referring to this specific scenario:

- FAN_CREATE mark exclusively placed on /dir1/old_file
- Create link(/dir1/old_file, /dir2/new_file)
- Expect to receive single event including two information records
  FID(/dir1/old_file) + DFID_NAME(/dir2/new_file)

Is that correct?

> In fact, if an application is watching FAN_RENAME to track the
> movement of a non-dir file and does not watch hardlink+unlink, then
> the file could escape under the application's nose.

That's understood.

/M

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

On Fri, May 13, 2022 at 4:18 PM Matthew Bobrowski <repnop@google.com> wrote:
>
> On Sat, May 07, 2022 at 07:03:13PM +0300, Amir Goldstein wrote:
> > Sorry Matthew, I was looking at the code to give you pointers, but there were
> > so many subtle details (as Jan has expected) that I could only communicate
> > them with a patch.
> > I tested that this patch does not break anything, but did not implement the
> > UAPI changes, so the functionality that it adds is not tested - I leave that
> > to you.
>
> No, that's totally fine. I had to familiarize myself with the
> FS/FAN_RENAME implementation as I hadn't gone over that series. So
> appreciate you whipping this together quickly as it would've taken a
> fair bit of time.
>
> Before the UAPI related modifications, we need to first figure out how
> we are to handle the CREATE/DELETE/MOVE cases.
>
> ...
>
> > My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> > of our design, this is because rename(2) is a snowflake vfs operation.
> > The event information simply reflects the operation complexity and when
> > looking at non-Linux filesystem event APIs, the event information for rename
> > looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> > was enhanced at some point exactly as we did with FAN_RENAME to
> > have all the info in one event vs. having to join two events.
> >
> > Hopefully, the attached patch simplifies the specialized implementation
> > a little bit.
> >
> > But... (there is always a but when it comes to UAPI),
> > When looking at my patch, one cannot help wondering -
> > what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> > If those can report child fid, why should they be treated differently
> > than FAN_RENAME w.r.t marking the child inode?
>
> This is something that crossed my mind while looking over the patch
> and is a very good thing to call-out indeed. I am of the opinion that
> we shouldn't be placing FAN_RENAME in the special egg basket and also
> consider how this is to operate for events
> FAN_CREATE/FAN_DELETE/FAN_MOVE.
>
> > For example, when watching a non-dir for FAN_CREATE, it could
> > be VERY helpful to get the dirfid+name of where the inode was
> > hard linked.
>
> Oh right, here you're referring to this specific scenario:
>
> - FAN_CREATE mark exclusively placed on /dir1/old_file
> - Create link(/dir1/old_file, /dir2/new_file)
> - Expect to receive single event including two information records
>   FID(/dir1/old_file) + DFID_NAME(/dir2/new_file)
>
> Is that correct?

Correct.
Exactly the same event as you would get from watching dir2 with
FAN_CREATE|FAN_EVENT_ON_CHILD in a group with flag
FAN_REPORT_TARGET_FID.

Thanks,
Amir.

Re: Fanotify API - Tracking File Movement

[Matthew Bobrowski]

On Fri, May 13, 2022 at 05:14:57PM +0300, Amir Goldstein wrote:
> On Fri, May 13, 2022 at 4:18 PM Matthew Bobrowski <repnop@google.com> wrote:
> >
> > On Sat, May 07, 2022 at 07:03:13PM +0300, Amir Goldstein wrote:
> > > Sorry Matthew, I was looking at the code to give you pointers, but there were
> > > so many subtle details (as Jan has expected) that I could only communicate
> > > them with a patch.
> > > I tested that this patch does not break anything, but did not implement the
> > > UAPI changes, so the functionality that it adds is not tested - I leave that
> > > to you.
> >
> > No, that's totally fine. I had to familiarize myself with the
> > FS/FAN_RENAME implementation as I hadn't gone over that series. So
> > appreciate you whipping this together quickly as it would've taken a
> > fair bit of time.
> >
> > Before the UAPI related modifications, we need to first figure out how
> > we are to handle the CREATE/DELETE/MOVE cases.
> >
> > ...
> >
> > > My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> > > of our design, this is because rename(2) is a snowflake vfs operation.
> > > The event information simply reflects the operation complexity and when
> > > looking at non-Linux filesystem event APIs, the event information for rename
> > > looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> > > was enhanced at some point exactly as we did with FAN_RENAME to
> > > have all the info in one event vs. having to join two events.
> > >
> > > Hopefully, the attached patch simplifies the specialized implementation
> > > a little bit.
> > >
> > > But... (there is always a but when it comes to UAPI),
> > > When looking at my patch, one cannot help wondering -
> > > what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> > > If those can report child fid, why should they be treated differently
> > > than FAN_RENAME w.r.t marking the child inode?
> >
> > This is something that crossed my mind while looking over the patch
> > and is a very good thing to call-out indeed. I am of the opinion that
> > we shouldn't be placing FAN_RENAME in the special egg basket and also
> > consider how this is to operate for events
> > FAN_CREATE/FAN_DELETE/FAN_MOVE.
> >
> > > For example, when watching a non-dir for FAN_CREATE, it could
> > > be VERY helpful to get the dirfid+name of where the inode was
> > > hard linked.
> >
> > Oh right, here you're referring to this specific scenario:
> >
> > - FAN_CREATE mark exclusively placed on /dir1/old_file
> > - Create link(/dir1/old_file, /dir2/new_file)
> > - Expect to receive single event including two information records
> >   FID(/dir1/old_file) + DFID_NAME(/dir2/new_file)
> >
> > Is that correct?
> 
> Correct.
> Exactly the same event as you would get from watching dir2 with
> FAN_CREATE|FAN_EVENT_ON_CHILD in a group with flag
> FAN_REPORT_TARGET_FID.

Right, that makes sense. For FAN_CREATE and FAN_DELETE (not entirely
sure about FAN_MOVE right now), as you mentioned can we simply provide
the DFID_NAME of the non-directory indirect objects? From a UAPI
perspective, I think in terms of what's expected in such situation
would be clear.

/M

Re: Fanotify API - Tracking File Movement

[Amir Goldstein]

On Fri, May 13, 2022 at 6:54 PM Matthew Bobrowski <repnop@google.com> wrote:
>
> On Fri, May 13, 2022 at 05:14:57PM +0300, Amir Goldstein wrote:
> > On Fri, May 13, 2022 at 4:18 PM Matthew Bobrowski <repnop@google.com> wrote:
> > >
> > > On Sat, May 07, 2022 at 07:03:13PM +0300, Amir Goldstein wrote:
> > > > Sorry Matthew, I was looking at the code to give you pointers, but there were
> > > > so many subtle details (as Jan has expected) that I could only communicate
> > > > them with a patch.
> > > > I tested that this patch does not break anything, but did not implement the
> > > > UAPI changes, so the functionality that it adds is not tested - I leave that
> > > > to you.
> > >
> > > No, that's totally fine. I had to familiarize myself with the
> > > FS/FAN_RENAME implementation as I hadn't gone over that series. So
> > > appreciate you whipping this together quickly as it would've taken a
> > > fair bit of time.
> > >
> > > Before the UAPI related modifications, we need to first figure out how
> > > we are to handle the CREATE/DELETE/MOVE cases.
> > >
> > > ...
> > >
> > > > My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> > > > of our design, this is because rename(2) is a snowflake vfs operation.
> > > > The event information simply reflects the operation complexity and when
> > > > looking at non-Linux filesystem event APIs, the event information for rename
> > > > looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> > > > was enhanced at some point exactly as we did with FAN_RENAME to
> > > > have all the info in one event vs. having to join two events.
> > > >
> > > > Hopefully, the attached patch simplifies the specialized implementation
> > > > a little bit.
> > > >
> > > > But... (there is always a but when it comes to UAPI),
> > > > When looking at my patch, one cannot help wondering -
> > > > what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> > > > If those can report child fid, why should they be treated differently
> > > > than FAN_RENAME w.r.t marking the child inode?
> > >
> > > This is something that crossed my mind while looking over the patch
> > > and is a very good thing to call-out indeed. I am of the opinion that
> > > we shouldn't be placing FAN_RENAME in the special egg basket and also
> > > consider how this is to operate for events
> > > FAN_CREATE/FAN_DELETE/FAN_MOVE.
> > >
> > > > For example, when watching a non-dir for FAN_CREATE, it could
> > > > be VERY helpful to get the dirfid+name of where the inode was
> > > > hard linked.
> > >
> > > Oh right, here you're referring to this specific scenario:
> > >
> > > - FAN_CREATE mark exclusively placed on /dir1/old_file
> > > - Create link(/dir1/old_file, /dir2/new_file)
> > > - Expect to receive single event including two information records
> > >   FID(/dir1/old_file) + DFID_NAME(/dir2/new_file)
> > >
> > > Is that correct?
> >
> > Correct.
> > Exactly the same event as you would get from watching dir2 with
> > FAN_CREATE|FAN_EVENT_ON_CHILD in a group with flag
> > FAN_REPORT_TARGET_FID.
>
> Right, that makes sense. For FAN_CREATE and FAN_DELETE (not entirely
> sure about FAN_MOVE right now),

FAN_MOVED_TO FAN_MOVED_FROM are not different than
FAN_CREATE FAN_DELETE they carry exact same info

> as you mentioned can we simply provide
> the DFID_NAME of the non-directory indirect objects? From a UAPI
> perspective, I think in terms of what's expected in such situation
> would be clear.
>

I think there may be some confusion.
We are not suggesting to change anything w.r.t the event info.
FAN_REPORT_TARGET_FID already defines that those dirent
events on non-dir with carry DFID_NAME and FID of non-dir child.

The only difference in behavior is that we are going to allow,
with group flag FAN_REPORT_TARGET_FID, to set the dirent
events in mask of a non-dir child to receive the exact same events
with same event info as received today for a watching parent.

Thanks,
Amir.