[PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Fabian Frederick]

ltp/fanotify02 was locked since commit 66ba93c0d7fe
("fanotify: don't set FAN_ONDIR implicitly on a marks ignored mask")

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/notify/fanotify/fanotify.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d2f97ec..7b3a50b 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -140,8 +140,8 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	}
 
 	if (d_is_dir(path->dentry) &&
-	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
-		return false;
+	    (marks_mask & FS_ISDIR & ~marks_ignored_mask))
+		return true;
 
 	if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask &
 				 ~marks_ignored_mask)
-- 
1.9.1

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Andrew Morton]

On Fri, 20 Mar 2015 21:56:08 +0100 Fabian Frederick <fabf@skynet.be> wrote:

> ltp/fanotify02 was locked since commit 66ba93c0d7fe
> ("fanotify: don't set FAN_ONDIR implicitly on a marks ignored mask")

What does "ltp/fanotify02 was locked" mean?  That this particular test
failed to exit, or...?

> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -140,8 +140,8 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  	}
>  
>  	if (d_is_dir(path->dentry) &&
> -	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> -		return false;
> +	    (marks_mask & FS_ISDIR & ~marks_ignored_mask))
> +		return true;

Should that be (marks_mask & FS_ISDIR & marks_ignored_mask)?

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Fabian Frederick]

> On 20 March 2015 at 22:09 Andrew Morton <akpm@linux-foundation.org> wrote:
>
>
> On Fri, 20 Mar 2015 21:56:08 +0100 Fabian Frederick <fabf@skynet.be> wrote:
>
> > ltp/fanotify02 was locked since commit 66ba93c0d7fe
> > ("fanotify: don't set FAN_ONDIR implicitly on a marks ignored mask")
>
> What does "ltp/fanotify02 was locked" mean?  That this particular test
> failed to exit, or...?
Hi Andrew,

   It failed to exit, exactly . No information displayed...
Problem appeared in 4.0-rc1.

Regards,
Fabian

>
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -140,8 +140,8 @@ static bool fanotify_should_send_event(struct
> > fsnotify_mark *inode_mark,
> >     }
> > 
> >     if (d_is_dir(path->dentry) &&
> > -       !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > -           return false;
> > +       (marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > +           return true;
>
> Should that be (marks_mask & FS_ISDIR & marks_ignored_mask)?

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Lino Sanfilippo]

On 20.03.2015 22:09, Andrew Morton wrote:
> On Fri, 20 Mar 2015 21:56:08 +0100 Fabian Frederick <fabf@skynet.be> wrote:
> 
>> ltp/fanotify02 was locked since commit 66ba93c0d7fe
>> ("fanotify: don't set FAN_ONDIR implicitly on a marks ignored mask")
> 
> What does "ltp/fanotify02 was locked" mean?  That this particular test
> failed to exit, or...?
> 
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -140,8 +140,8 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>>  	}
>>  
>>  	if (d_is_dir(path->dentry) &&
>> -	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
>> -		return false;
>> +	    (marks_mask & FS_ISDIR & ~marks_ignored_mask))
>> +		return true;
> 
> Should that be (marks_mask & FS_ISDIR & marks_ignored_mask)?
> 

No, the current logic should be correct, since we want events for
directories if we have FS_ISDIR set in the marks mask but not in its
ignored_mask.

Regards,
Lino

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Lino Sanfilippo]

On 21.03.2015 02:01, Lino Sanfilippo wrote:

>> Should that be (marks_mask & FS_ISDIR & marks_ignored_mask)?
>> 
> 
> No, the current logic should be correct, since we want events for
> directories if we have FS_ISDIR set in the marks mask but not in its
> ignored_mask.
> 

Actually this should be: "... since we ONLY want events for directories
if we have FS_ISDIR set in the marks mask but not in its ignored_mask".

With Fabians Code we could even get events for dirs although FAN_ONDIR
has not been set - which is not what we want.

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Fabian Frederick]

> On 21 March 2015 at 02:15 Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:
>
>
> On 21.03.2015 02:01, Lino Sanfilippo wrote:
>
> >> Should that be (marks_mask & FS_ISDIR & marks_ignored_mask)?
> >>
> >
> > No, the current logic should be correct, since we want events for
> > directories if we have FS_ISDIR set in the marks mask but not in its
> > ignored_mask.
> >
>
> Actually this should be: "... since we ONLY want events for directories
> if we have FS_ISDIR set in the marks mask but not in its ignored_mask".
>
> With Fabians Code we could even get events for dirs although FAN_ONDIR
> has not been set - which is not what we want.
>
Let's hope it only breaks ltp tests and no _real_ userland stuff
(search systems ...)

Regards,
Fabian

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Lino Sanfilippo]

On 22.03.2015 10:46, Fabian Frederick wrote:

> Let's hope it only breaks ltp tests and no _real_ userland stuff
> (search systems ...)
> 
> Regards,
> Fabian
> 

Hi Fabian,

yes, that worries me too. I know that there have been discussions on
lkml in which it was made clear that userspace breakage is frowned upon.
And it is obvious, that the latest changes concerning the handling of
FAN_ONDIR are also visible to userspace. But since the concerning
patches have been accepted I think it is ok. I could be wrong though.

Maybe someone with a deeper knowledge of kernel policy/guidelines could
comment on this?

Regards,
Lino

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Jan Kara]

On Sun 22-03-15 11:48:20, Lino Sanfilippo wrote:
> On 22.03.2015 10:46, Fabian Frederick wrote:
> 
> > Let's hope it only breaks ltp tests and no _real_ userland stuff
> > (search systems ...)
> > 
> > Regards,
> > Fabian
> > 
> 
> Hi Fabian,
> 
> yes, that worries me too. I know that there have been discussions on
> lkml in which it was made clear that userspace breakage is frowned upon.
> And it is obvious, that the latest changes concerning the handling of
> FAN_ONDIR are also visible to userspace. But since the concerning
> patches have been accepted I think it is ok. I could be wrong though.
> 
> Maybe someone with a deeper knowledge of kernel policy/guidelines could
> comment on this?
  For now I'd stay calm. Events on directories with fanotify are hopelessly
useless anyway so I doubt anyone uses them. I just tested those in fanotify
LTP testcase for completeness (and screwed that up as you noted). If we
find a real application that tries to use directory events with fanotify
and which is broken by this patch, we'll have to revert it, that's for
sure.

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

Re: [PATCH 1/1 linux-next] fanotify: fix a lock in fanotify_should_send_event()

[Lino Sanfilippo]

Hi,

On 20.03.2015 21:56, Fabian Frederick wrote:
> ltp/fanotify02 was locked since commit 66ba93c0d7fe
> ("fanotify: don't set FAN_ONDIR implicitly on a marks ignored mask")
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
>  fs/notify/fanotify/fanotify.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index d2f97ec..7b3a50b 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -140,8 +140,8 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  	}
>  
>  	if (d_is_dir(path->dentry) &&
> -	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> -		return false;
> +	    (marks_mask & FS_ISDIR & ~marks_ignored_mask))
> +		return true;
>  
>  	if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask &
>  				 ~marks_ignored_mask)
> 

I dont think that this is an issue in kernel code. The test code tries
to get events for the current directory but never sets the FAN_ONDIR
flag for the concerning mark - so it hangs because it is waiting for
events that are never generated. Adding FAN_ONDIR to the set of mark
flags should fix the test code.

Regards,
Lino