[PATCH 1/3] fanotify: only destroy mark when both mask and ignored_mask are cleared

[PATCH 1/3] fanotify: only destroy mark when both mask and ignored_mask are cleared

[Lino Sanfilippo]

In fanotify_mark_remove_from_mask() a mark is destroyed if only one of both
bitmasks (mask or ignored_mask) of a mark is cleared. However the other mask
may still be set and contain information that should not be lost. Thus only
destroy a mark if both masks are cleared.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 fs/notify/fanotify/fanotify_user.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c991616..03a0dd1 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -488,6 +488,8 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 					    int *destroy)
 {
 	__u32 oldmask;
+	__u32 new_mask;
+	__u32 new_ignored;
 
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
@@ -497,9 +499,11 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 		oldmask = fsn_mark->ignored_mask;
 		fsnotify_set_mark_ignored_mask_locked(fsn_mark, (oldmask & ~mask));
 	}
+	new_mask = fsn_mark->mask;
+	new_ignored = fsn_mark->ignored_mask;
 	spin_unlock(&fsn_mark->lock);
 
-	*destroy = !(oldmask & ~mask);
+	*destroy = !(new_mask | new_ignored);
 
 	return mask & oldmask;
 }
-- 
1.9.1

[PATCH 2/3] fanotify: dont recalculate a marks mask if only the ignored mask changed

[Lino Sanfilippo]

If removing bits from a marks ignored mask, the concerning inodes/vfsmounts
mask is not affected. So dont recalculate it.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 fs/notify/fanotify/fanotify_user.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 03a0dd1..3afd8bb 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -487,7 +487,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 					    unsigned int flags,
 					    int *destroy)
 {
-	__u32 oldmask;
+	__u32 oldmask = 0;
 	__u32 new_mask;
 	__u32 new_ignored;
 
@@ -496,8 +496,8 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 		oldmask = fsn_mark->mask;
 		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask & ~mask));
 	} else {
-		oldmask = fsn_mark->ignored_mask;
-		fsnotify_set_mark_ignored_mask_locked(fsn_mark, (oldmask & ~mask));
+		__u32 tmask = fsn_mark->ignored_mask & ~mask;
+		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
 	}
 	new_mask = fsn_mark->mask;
 	new_ignored = fsn_mark->ignored_mask;
-- 
1.9.1

Re: [PATCH 2/3] fanotify: dont recalculate a marks mask if only the ignored mask changed

[Jan Kara]

On Sun 30-11-14 00:37:37, Lino Sanfilippo wrote:
> If removing bits from a marks ignored mask, the concerning inodes/vfsmounts
> mask is not affected. So dont recalculate it.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/notify/fanotify/fanotify_user.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 03a0dd1..3afd8bb 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -487,7 +487,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>  					    unsigned int flags,
>  					    int *destroy)
>  {
> -	__u32 oldmask;
> +	__u32 oldmask = 0;
>  	__u32 new_mask;
>  	__u32 new_ignored;
>  
> @@ -496,8 +496,8 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>  		oldmask = fsn_mark->mask;
>  		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask & ~mask));
>  	} else {
> -		oldmask = fsn_mark->ignored_mask;
> -		fsnotify_set_mark_ignored_mask_locked(fsn_mark, (oldmask & ~mask));
> +		__u32 tmask = fsn_mark->ignored_mask & ~mask;
> +		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
>  	}
>  	new_mask = fsn_mark->mask;
>  	new_ignored = fsn_mark->ignored_mask;
> -- 
> 1.9.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 3/3] fanotify: fix inconsistent behaviour regarding flag FAN_ONDIR

[Lino Sanfilippo]

Before flag FAN_ONDIR was implicitly set in a marks ignored mask. This led to
some inconsistent behaviour:

1. It was not possible to remove the flag from the ignored mask, once it was set
(implicitly) with a call like

fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN, AT_FDCWD, "dir");

This was since the needed flag FAN_MARK_ONDIR was only honored when setting a
marks mask, but not when removing it. Now FAN_ONDIR is only set when explicitly
passed in the masks parameter. It now is also possible to remove it again:

fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
fanotify_mark(fd, FAN_MARK_REMOVE, FAN_ONDIR , AT_FDCWD, "dir");

2. Subsequent calls to fanotify_mark for a mark that had FAN_ONDIR already set
in its mask removed the flag, if it was not specified in the mask parameter
again. Thus

fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
fanotify_mark(fd, FAN_MARK_ADD, FAN_CLOSE, AT_FDCWD, "dir");

set FAN_ONDIR in the first call on the marks mask but also on the ignored
mask in the second call. So the first request for DIR events was overwritten.
Since the flag is now not set implicitly any longer this cant happen any more.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 fs/notify/fanotify/fanotify.c      |  2 +-
 fs/notify/fanotify/fanotify_user.c | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 30d3add..51ceb81 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -140,7 +140,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	}
 
 	if (S_ISDIR(path->dentry->d_inode->i_mode) &&
-	    (marks_ignored_mask & FS_ISDIR))
+	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return false;
 
 	if (event_mask & marks_mask & ~marks_ignored_mask)
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3afd8bb..e62463e 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -493,10 +493,17 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
+		__u32 tmask = fsn_mark->mask & ~mask;
+		if (flags & FAN_MARK_ONDIR)
+			tmask &= ~FAN_ONDIR;
+
 		oldmask = fsn_mark->mask;
-		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask & ~mask));
+		fsnotify_set_mark_mask_locked(fsn_mark, tmask);
 	} else {
 		__u32 tmask = fsn_mark->ignored_mask & ~mask;
+		if (flags & FAN_MARK_ONDIR)
+			tmask &= ~FAN_ONDIR;
+
 		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
 	}
 	new_mask = fsn_mark->mask;
@@ -573,20 +580,21 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
+		__u32 tmask = fsn_mark->mask | mask;
+		if (flags & FAN_MARK_ONDIR)
+			tmask |= FAN_ONDIR;
+
 		oldmask = fsn_mark->mask;
-		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask | mask));
+		fsnotify_set_mark_mask_locked(fsn_mark, tmask);
 	} else {
 		__u32 tmask = fsn_mark->ignored_mask | mask;
+		if (flags & FAN_MARK_ONDIR)
+			tmask |= FAN_ONDIR;
+
 		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
 		if (flags & FAN_MARK_IGNORED_SURV_MODIFY)
 			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
 	}
-
-	if (!(flags & FAN_MARK_ONDIR)) {
-		__u32 tmask = fsn_mark->ignored_mask | FAN_ONDIR;
-		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
-	}
-
 	spin_unlock(&fsn_mark->lock);
 
 	return mask & ~oldmask;
-- 
1.9.1

Re: [PATCH 3/3] fanotify: fix inconsistent behaviour regarding flag FAN_ONDIR

[Jan Kara]

On Sun 30-11-14 00:37:38, Lino Sanfilippo wrote:
> Before flag FAN_ONDIR was implicitly set in a marks ignored mask. This led to
> some inconsistent behaviour:
> 
> 1. It was not possible to remove the flag from the ignored mask, once it was set
> (implicitly) with a call like
> 
> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN, AT_FDCWD, "dir");
> 
> This was since the needed flag FAN_MARK_ONDIR was only honored when setting a
> marks mask, but not when removing it. Now FAN_ONDIR is only set when explicitly
> passed in the masks parameter. It now is also possible to remove it again:
> 
> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
> fanotify_mark(fd, FAN_MARK_REMOVE, FAN_ONDIR , AT_FDCWD, "dir");
> 
> 2. Subsequent calls to fanotify_mark for a mark that had FAN_ONDIR already set
> in its mask removed the flag, if it was not specified in the mask parameter
> again. Thus
> 
> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
> fanotify_mark(fd, FAN_MARK_ADD, FAN_CLOSE, AT_FDCWD, "dir");
> 
> set FAN_ONDIR in the first call on the marks mask but also on the ignored
> mask in the second call. So the first request for DIR events was overwritten.
> Since the flag is now not set implicitly any longer this cant happen any more.
  Ugh, I have to say I don't understand the changelog. From the code I
think I understood that instead of always adding FAN_ONDIR to ignore mask
you require FAN_ONDIR to be set in the normal mask. That makes sense to me
but please make the changelog more comprehensible. Also please add a
testcase to LTP fanotify() coverage for FAN_ONDIR behavior so that we can
easily see how userspace visible behavior changed / didn't change. Thanks!

								Honza
 

> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  fs/notify/fanotify/fanotify.c      |  2 +-
>  fs/notify/fanotify/fanotify_user.c | 24 ++++++++++++++++--------
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 30d3add..51ceb81 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -140,7 +140,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  	}
>  
>  	if (S_ISDIR(path->dentry->d_inode->i_mode) &&
> -	    (marks_ignored_mask & FS_ISDIR))
> +	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
>  		return false;
>  
>  	if (event_mask & marks_mask & ~marks_ignored_mask)
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 3afd8bb..e62463e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -493,10 +493,17 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>  
>  	spin_lock(&fsn_mark->lock);
>  	if (!(flags & FAN_MARK_IGNORED_MASK)) {
> +		__u32 tmask = fsn_mark->mask & ~mask;
> +		if (flags & FAN_MARK_ONDIR)
> +			tmask &= ~FAN_ONDIR;
> +
>  		oldmask = fsn_mark->mask;
> -		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask & ~mask));
> +		fsnotify_set_mark_mask_locked(fsn_mark, tmask);
>  	} else {
>  		__u32 tmask = fsn_mark->ignored_mask & ~mask;
> +		if (flags & FAN_MARK_ONDIR)
> +			tmask &= ~FAN_ONDIR;
> +
>  		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
>  	}
>  	new_mask = fsn_mark->mask;
> @@ -573,20 +580,21 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
>  
>  	spin_lock(&fsn_mark->lock);
>  	if (!(flags & FAN_MARK_IGNORED_MASK)) {
> +		__u32 tmask = fsn_mark->mask | mask;
> +		if (flags & FAN_MARK_ONDIR)
> +			tmask |= FAN_ONDIR;
> +
>  		oldmask = fsn_mark->mask;
> -		fsnotify_set_mark_mask_locked(fsn_mark, (oldmask | mask));
> +		fsnotify_set_mark_mask_locked(fsn_mark, tmask);
>  	} else {
>  		__u32 tmask = fsn_mark->ignored_mask | mask;
> +		if (flags & FAN_MARK_ONDIR)
> +			tmask |= FAN_ONDIR;
> +
>  		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
>  		if (flags & FAN_MARK_IGNORED_SURV_MODIFY)
>  			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
>  	}
> -
> -	if (!(flags & FAN_MARK_ONDIR)) {
> -		__u32 tmask = fsn_mark->ignored_mask | FAN_ONDIR;
> -		fsnotify_set_mark_ignored_mask_locked(fsn_mark, tmask);
> -	}
> -
>  	spin_unlock(&fsn_mark->lock);
>  
>  	return mask & ~oldmask;
> -- 
> 1.9.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 3/3] fanotify: fix inconsistent behaviour regarding flag FAN_ONDIR

[Lino Sanfilippo]

Hi,

On 01.12.2014 11:05, Jan Kara wrote:
> On Sun 30-11-14 00:37:38, Lino Sanfilippo wrote:
>> Before flag FAN_ONDIR was implicitly set in a marks ignored mask. This led to
>> some inconsistent behaviour:
>> 
>> 1. It was not possible to remove the flag from the ignored mask, once it was set
>> (implicitly) with a call like
>> 
>> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN, AT_FDCWD, "dir");
>> 
>> This was since the needed flag FAN_MARK_ONDIR was only honored when setting a
>> marks mask, but not when removing it. Now FAN_ONDIR is only set when explicitly
>> passed in the masks parameter. It now is also possible to remove it again:
>> 
>> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
>> fanotify_mark(fd, FAN_MARK_REMOVE, FAN_ONDIR , AT_FDCWD, "dir");
>> 
>> 2. Subsequent calls to fanotify_mark for a mark that had FAN_ONDIR already set
>> in its mask removed the flag, if it was not specified in the mask parameter
>> again. Thus
>> 
>> fanotify_mark(fd, FAN_MARK_ADD, FAN_OPEN | FAN_ONDIR , AT_FDCWD, "dir");
>> fanotify_mark(fd, FAN_MARK_ADD, FAN_CLOSE, AT_FDCWD, "dir");
>> 
>> set FAN_ONDIR in the first call on the marks mask but also on the ignored
>> mask in the second call. So the first request for DIR events was overwritten.
>> Since the flag is now not set implicitly any longer this cant happen any more.
>   Ugh, I have to say I don't understand the changelog. From the code I
> think I understood that instead of always adding FAN_ONDIR to ignore mask
> you require FAN_ONDIR to be set in the normal mask. That makes sense to me
> but please make the changelog more comprehensible.

Hm. My intention was to describe at least two cases of strange behaviour
when setting the FAN_ONDIR flag. But maybe the description has indeed
become a little bit too complicated. I will adjust that and resend the
patch.

 Also please add a
> testcase to LTP fanotify() coverage for FAN_ONDIR behavior so that we can
> easily see how userspace visible behavior changed / didn't change. Thanks!
> 

Ok, i can do that (it may take a few days until i find the time for it
though). Thank you for review!

Regards,
Lino

Re: [PATCH 1/3] fanotify: only destroy mark when both mask and ignored_mask are cleared

[Jan Kara]

On Sun 30-11-14 00:37:36, Lino Sanfilippo wrote:
> In fanotify_mark_remove_from_mask() a mark is destroyed if only one of both
> bitmasks (mask or ignored_mask) of a mark is cleared. However the other mask
> may still be set and contain information that should not be lost. Thus only
> destroy a mark if both masks are cleared.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  fs/notify/fanotify/fanotify_user.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index c991616..03a0dd1 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -488,6 +488,8 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>  					    int *destroy)
>  {
>  	__u32 oldmask;
> +	__u32 new_mask;
> +	__u32 new_ignored;
>  
>  	spin_lock(&fsn_mark->lock);
>  	if (!(flags & FAN_MARK_IGNORED_MASK)) {
> @@ -497,9 +499,11 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>  		oldmask = fsn_mark->ignored_mask;
>  		fsnotify_set_mark_ignored_mask_locked(fsn_mark, (oldmask & ~mask));
>  	}
> +	new_mask = fsn_mark->mask;
> +	new_ignored = fsn_mark->ignored_mask;
>  	spin_unlock(&fsn_mark->lock);
>  
> -	*destroy = !(oldmask & ~mask);
> +	*destroy = !(new_mask | new_ignored);
  There's no need for new variables, is there? You can just set *destroy
under the spinlock...

								Honza

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

Re: [PATCH 1/3] fanotify: only destroy mark when both mask and ignored_mask are cleared

[Lino Sanfilippo]

Hi,

On 01.12.2014 10:04, Jan Kara wrote:
> On Sun 30-11-14 00:37:36, Lino Sanfilippo wrote:
>> In fanotify_mark_remove_from_mask() a mark is destroyed if only one of both
>> bitmasks (mask or ignored_mask) of a mark is cleared. However the other mask
>> may still be set and contain information that should not be lost. Thus only
>> destroy a mark if both masks are cleared.
>> 
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>  fs/notify/fanotify/fanotify_user.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index c991616..03a0dd1 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -488,6 +488,8 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>>  					    int *destroy)
>>  {
>>  	__u32 oldmask;
>> +	__u32 new_mask;
>> +	__u32 new_ignored;
>>  
>>  	spin_lock(&fsn_mark->lock);
>>  	if (!(flags & FAN_MARK_IGNORED_MASK)) {
>> @@ -497,9 +499,11 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
>>  		oldmask = fsn_mark->ignored_mask;
>>  		fsnotify_set_mark_ignored_mask_locked(fsn_mark, (oldmask & ~mask));
>>  	}
>> +	new_mask = fsn_mark->mask;
>> +	new_ignored = fsn_mark->ignored_mask;
>>  	spin_unlock(&fsn_mark->lock);
>>  
>> -	*destroy = !(oldmask & ~mask);
>> +	*destroy = !(new_mask | new_ignored);
>   There's no need for new variables, is there? You can just set *destroy
> under the spinlock...
> 

youre right, these variables are totally unneeded. I cant remember the
reason why i introduced them (maybe leftovers from an earlier attempt).
I will resend a cleaned up version of that patch.

Regards,
Lino