The fanotify and the inotify API can be used to monitor changes of the file
system.
System call ftruncate modifies files. Hence it should trigger the corresponding
fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events.
Signed-off-by: Heinrich Schuchardt <[email protected]>
---
fs/open.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index d6fd3ac..e36f26e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
error = security_path_truncate(&f.file->f_path);
if (!error)
error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
+ if (!error)
+ fsnotify_modify(f.file);
sb_end_write(inode->i_sb);
out_putf:
fdput(f);
--
2.1.0
On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote:
> The fanotify and the inotify API can be used to monitor changes of the file
> system.
>
> System call ftruncate modifies files. Hence it should trigger the corresponding
> fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events.
Hum, I would think that the appropriate event gets generated by
fsnotify_change() called from notify_change()?
Honza
>
> Signed-off-by: Heinrich Schuchardt <[email protected]>
> ---
> fs/open.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/open.c b/fs/open.c
> index d6fd3ac..e36f26e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> error = security_path_truncate(&f.file->f_path);
> if (!error)
> error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
> + if (!error)
> + fsnotify_modify(f.file);
> sb_end_write(inode->i_sb);
> out_putf:
> fdput(f);
> --
> 2.1.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 06.10.2014 15:24, Jan Kara wrote:
> On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote:
>> The fanotify and the inotify API can be used to monitor changes of the file
>> system.
>>
>> System call ftruncate modifies files. Hence it should trigger the corresponding
>> fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events.
> Hum, I would think that the appropriate event gets generated by
> fsnotify_change() called from notify_change()?
>
> Honza
Hello Jan,
thank you for reviewing.
fsnotify_change() calls
fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
fsnotify_modify() calls
fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
Only with FSNOTIFY_EVENT_PATH a mount is determined in fsnotify().
So a FAN_MODIFY event for a mount cannot occur. But I also did not see a
FAN_MODIFY event when watching a file either.
IN_MODIFY events are actually created.
So would it be better to do the change in do_truncate and replace the
call to notify_change by a call to fsnotify_modify?
I just had a look at
http://lxr.free-electrons.com/ident?i=notify_change
There seem to be some other usages of notify_change that deserve
consideration, e.g. in ecryptfs_truncate().
Best regards
Heinrich Schuchardt
>>
>> Signed-off-by: Heinrich Schuchardt <[email protected]>
>> ---
>> fs/open.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/open.c b/fs/open.c
>> index d6fd3ac..e36f26e 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>> error = security_path_truncate(&f.file->f_path);
>> if (!error)
>> error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
>> + if (!error)
>> + fsnotify_modify(f.file);
>> sb_end_write(inode->i_sb);
>> out_putf:
>> fdput(f);
>> --
>> 2.1.0
>>
On Mon 06-10-14 22:09:12, Heinrich Schuchardt wrote:
> On 06.10.2014 15:24, Jan Kara wrote:
> >On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote:
> >>The fanotify and the inotify API can be used to monitor changes of the file
> >>system.
> >>
> >>System call ftruncate modifies files. Hence it should trigger the corresponding
> >>fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events.
> > Hum, I would think that the appropriate event gets generated by
> >fsnotify_change() called from notify_change()?
> >
> > Honza
>
> Hello Jan,
>
> thank you for reviewing.
>
> fsnotify_change() calls
> fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
>
> fsnotify_modify() calls
> fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
>
> Only with FSNOTIFY_EVENT_PATH a mount is determined in fsnotify().
>
> So a FAN_MODIFY event for a mount cannot occur. But I also did not
> see a FAN_MODIFY event when watching a file either.
Yeah, fanotify processes only FSNOTIFY_EVENT_PATH events (without a path
it cannot create an fd to pass to userspace).
> IN_MODIFY events are actually created.
>
> So would it be better to do the change in do_truncate and replace
> the call to notify_change by a call to fsnotify_modify?
>
> I just had a look at
> http://lxr.free-electrons.com/ident?i=notify_change
>
> There seem to be some other usages of notify_change that deserve
> consideration, e.g. in ecryptfs_truncate().
Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in
notify_change() is because we have only dentry available there. OTOH from a
quick look it doesn't look impossible to pass path there from the callers.
So I'd rather propagate path to notify_change() and generate also fanotify
events there than generating truncate events for fanotify separately
somewhere else...
Honza
> >>Signed-off-by: Heinrich Schuchardt <[email protected]>
> >>---
> >> fs/open.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >>diff --git a/fs/open.c b/fs/open.c
> >>index d6fd3ac..e36f26e 100644
> >>--- a/fs/open.c
> >>+++ b/fs/open.c
> >>@@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> >> error = security_path_truncate(&f.file->f_path);
> >> if (!error)
> >> error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
> >>+ if (!error)
> >>+ fsnotify_modify(f.file);
> >> sb_end_write(inode->i_sb);
> >> out_putf:
> >> fdput(f);
> >>--
> >>2.1.0
> >>
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
:: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote:
::
:: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in
:: notify_change() is because we have only dentry available there. OTOH from a
:: quick look it doesn't look impossible to pass path there from the callers.
:: So I'd rather propagate path to notify_change() and generate also fanotify
:: events there than generating truncate events for fanotify separately
:: somewhere else...
The attached second version of the patch follows this idea of Jan.
The fanotify and fanotify API can be used to monitor changes of the file
system.
System calls ftruncate and truncate modify files. Hence they should trigger
the corresponding fanotify (FAN_MODIFY) event.
Prior to the patch only a inotify (IN_MODIFY) event is triggered because
the path information is not passed to the notification framework.
The patch changes the interface of fsnotify_change to allow path information
to be passed. Fsnotify_change is only called by notify_change and by
fat_ioctl_set_attributes.
notify_change is called by a larger number of callers. Not all of these
have access to path object. To limit the number of changes the patch renames
notify_change to do_notify_change and adds a path parameter. The patch further
adds two wrappers: notify_change for callers that cannot provide a path
and notify_change_path for callers that can provide a path.
The patch changes do_truncate to accept a path parameter passed to
notify_change_path and adjusts all callers of do_truncate.
Signed-off-by: Heinrich Schuchardt <[email protected]>
---
fs/attr.c | 35 ++++++++++++++++++++++++++++++++---
fs/fat/file.c | 2 +-
fs/namei.c | 2 +-
fs/open.c | 10 ++++++----
include/linux/fs.h | 3 ++-
include/linux/fsnotify.h | 12 +++++++++---
6 files changed, 51 insertions(+), 13 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index 6530ced..894967e 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
EXPORT_SYMBOL(setattr_copy);
/**
- * notify_change - modify attributes of a filesytem object
+ * do_notify_change - modify attributes of a filesytem object
+ * @path: path of object affected
* @dentry: object affected
* @iattr: new attributes
* @delegated_inode: returns inode, if the inode is delegated
*
* The caller must hold the i_mutex on the affected object.
*
+ * If NULL is passed in attribute path, fanotify events cannot be
+ * created.
+ *
* If notify_change discovers a delegation in need of breaking,
* it will return -EWOULDBLOCK and return a reference to the inode in
* delegated_inode. The caller should then break the delegation and
@@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy);
* the file open for write, as there can be no conflicting delegation in
* that case.
*/
-int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode)
+int do_notify_change(struct path *path, struct dentry *dentry,
+ struct iattr *attr, struct inode **delegated_inode)
{
struct inode *inode = dentry->d_inode;
umode_t mode = inode->i_mode;
@@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
error = simple_setattr(dentry, attr);
if (!error) {
- fsnotify_change(dentry, ia_valid);
+ fsnotify_change(path, dentry, ia_valid);
ima_inode_post_setattr(dentry);
evm_inode_post_setattr(dentry, ia_valid);
}
return error;
}
+
+/*
+ * notify_change - modify attributes of a filesytem object
+ *
+ * This call will not create fanotify events. It should be replaced by
+ * notify_change_path where possible.
+ */
+int notify_change(struct dentry *dentry, struct iattr *attr,
+ struct inode **delegated_inode)
+{
+ return do_notify_change(NULL, dentry, attr, delegated_inode);
+}
EXPORT_SYMBOL(notify_change);
+
+/*
+ * notify_change_path - modify attributes of a filesytem object
+ *
+ * This call is a replacement for notify_change. It creates fanotify events.
+ */
+int notify_change_path(struct path *path, struct iattr *attr,
+ struct inode **delegated_inode)
+{
+ return do_notify_change(path, path->dentry, attr, delegated_inode);
+}
+EXPORT_SYMBOL(notify_change_path);
diff --git a/fs/fat/file.c b/fs/fat/file.c
index f2c73ae..d293a0b 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
if (err)
goto out_unlock_inode;
- fsnotify_change(file->f_path.dentry, ia.ia_valid);
+ fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid);
if (sbi->options.sys_immutable) {
if (attr & ATTR_SYS)
inode->i_flags |= S_IMMUTABLE;
diff --git a/fs/namei.c b/fs/namei.c
index 43927d1..0a1f3b7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp)
if (!error)
error = security_path_truncate(path);
if (!error) {
- error = do_truncate(path->dentry, 0,
+ error = do_truncate(path, 0,
ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
filp);
}
diff --git a/fs/open.c b/fs/open.c
index d6fd3ac..390e322 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -34,11 +34,12 @@
#include "internal.h"
-int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
+int do_truncate(struct path *path, loff_t length, unsigned int time_attrs,
struct file *filp)
{
int ret;
struct iattr newattrs;
+ struct dentry *dentry = path->dentry;
/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
if (length < 0)
@@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
mutex_lock(&dentry->d_inode->i_mutex);
/* Note any delegations or leases have already been broken: */
- ret = notify_change(dentry, &newattrs, NULL);
+ ret = notify_change_path(path, &newattrs, NULL);
mutex_unlock(&dentry->d_inode->i_mutex);
return ret;
}
@@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length)
if (!error)
error = security_path_truncate(path);
if (!error)
- error = do_truncate(path->dentry, length, 0, NULL);
+ error = do_truncate(path, length, 0, NULL);
put_write_and_out:
put_write_access(inode);
@@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
if (!error)
error = security_path_truncate(&f.file->f_path);
if (!error)
- error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
+ error = do_truncate(&f.file->f_path, length,
+ ATTR_MTIME|ATTR_CTIME, f.file);
sb_end_write(inode->i_sb);
out_putf:
fdput(f);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4111ee0..e5b0b05 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2031,7 +2031,7 @@ struct filename {
};
extern long vfs_truncate(struct path *, loff_t);
-extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
+extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs,
struct file *filp);
extern int do_fallocate(struct file *file, int mode, loff_t offset,
loff_t len);
@@ -2253,6 +2253,7 @@ extern void emergency_remount(void);
extern sector_t bmap(struct inode *, sector_t);
#endif
extern int notify_change(struct dentry *, struct iattr *, struct inode **);
+extern int notify_change_path(struct path *, struct iattr *, struct inode **);
extern int inode_permission(struct inode *, int);
extern int generic_permission(struct inode *, int);
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 1c804b0..0fd594b 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry)
* fsnotify_change - notify_change event. file was modified and/or metadata
* was changed.
*/
-static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
+static inline void fsnotify_change(struct path *path, struct dentry *dentry,
+ unsigned int ia_valid)
{
struct inode *inode = dentry->d_inode;
__u32 mask = 0;
@@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
if (S_ISDIR(inode->i_mode))
mask |= FS_ISDIR;
- fsnotify_parent(NULL, dentry, mask);
- fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+ fsnotify_parent(path, dentry, mask);
+ if (path == NULL)
+ fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE,
+ NULL, 0);
+ else
+ fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH,
+ NULL, 0);
}
}
--
2.1.1
On Thu 23-10-14 23:35:07, Heinrich Schuchardt wrote:
> :: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote:
> ::
> :: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in
> :: notify_change() is because we have only dentry available there. OTOH from a
> :: quick look it doesn't look impossible to pass path there from the callers.
> :: So I'd rather propagate path to notify_change() and generate also fanotify
> :: events there than generating truncate events for fanotify separately
> :: somewhere else...
>
> The attached second version of the patch follows this idea of Jan.
>
> The fanotify and fanotify API can be used to monitor changes of the file
> system.
>
> System calls ftruncate and truncate modify files. Hence they should trigger
> the corresponding fanotify (FAN_MODIFY) event.
>
> Prior to the patch only a inotify (IN_MODIFY) event is triggered because
> the path information is not passed to the notification framework.
>
> The patch changes the interface of fsnotify_change to allow path information
> to be passed. Fsnotify_change is only called by notify_change and by
> fat_ioctl_set_attributes.
>
> notify_change is called by a larger number of callers. Not all of these
> have access to path object. To limit the number of changes the patch renames
> notify_change to do_notify_change and adds a path parameter. The patch further
> adds two wrappers: notify_change for callers that cannot provide a path
> and notify_change_path for callers that can provide a path.
>
> The patch changes do_truncate to accept a path parameter passed to
> notify_change_path and adjusts all callers of do_truncate.
So what I somewhat dislike about this patch is that notify_change() is
sometimes called with dentry and sometimes with path. That way it's not
completely clear when fanotify events will be generated and when not.
Sadly it isn't easy to provide struct path in all the places where we are
calling notify_change() so I'm not sure what would a better solution look
like either :(
Honza
>
> Signed-off-by: Heinrich Schuchardt <[email protected]>
> ---
> fs/attr.c | 35 ++++++++++++++++++++++++++++++++---
> fs/fat/file.c | 2 +-
> fs/namei.c | 2 +-
> fs/open.c | 10 ++++++----
> include/linux/fs.h | 3 ++-
> include/linux/fsnotify.h | 12 +++++++++---
> 6 files changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 6530ced..894967e 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
> EXPORT_SYMBOL(setattr_copy);
>
> /**
> - * notify_change - modify attributes of a filesytem object
> + * do_notify_change - modify attributes of a filesytem object
> + * @path: path of object affected
> * @dentry: object affected
> * @iattr: new attributes
> * @delegated_inode: returns inode, if the inode is delegated
> *
> * The caller must hold the i_mutex on the affected object.
> *
> + * If NULL is passed in attribute path, fanotify events cannot be
> + * created.
> + *
> * If notify_change discovers a delegation in need of breaking,
> * it will return -EWOULDBLOCK and return a reference to the inode in
> * delegated_inode. The caller should then break the delegation and
> @@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy);
> * the file open for write, as there can be no conflicting delegation in
> * that case.
> */
> -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode)
> +int do_notify_change(struct path *path, struct dentry *dentry,
> + struct iattr *attr, struct inode **delegated_inode)
> {
> struct inode *inode = dentry->d_inode;
> umode_t mode = inode->i_mode;
> @@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> error = simple_setattr(dentry, attr);
>
> if (!error) {
> - fsnotify_change(dentry, ia_valid);
> + fsnotify_change(path, dentry, ia_valid);
> ima_inode_post_setattr(dentry);
> evm_inode_post_setattr(dentry, ia_valid);
> }
>
> return error;
> }
> +
> +/*
> + * notify_change - modify attributes of a filesytem object
> + *
> + * This call will not create fanotify events. It should be replaced by
> + * notify_change_path where possible.
> + */
> +int notify_change(struct dentry *dentry, struct iattr *attr,
> + struct inode **delegated_inode)
> +{
> + return do_notify_change(NULL, dentry, attr, delegated_inode);
> +}
> EXPORT_SYMBOL(notify_change);
> +
> +/*
> + * notify_change_path - modify attributes of a filesytem object
> + *
> + * This call is a replacement for notify_change. It creates fanotify events.
> + */
> +int notify_change_path(struct path *path, struct iattr *attr,
> + struct inode **delegated_inode)
> +{
> + return do_notify_change(path, path->dentry, attr, delegated_inode);
> +}
> +EXPORT_SYMBOL(notify_change_path);
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index f2c73ae..d293a0b 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
> if (err)
> goto out_unlock_inode;
>
> - fsnotify_change(file->f_path.dentry, ia.ia_valid);
> + fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid);
> if (sbi->options.sys_immutable) {
> if (attr & ATTR_SYS)
> inode->i_flags |= S_IMMUTABLE;
> diff --git a/fs/namei.c b/fs/namei.c
> index 43927d1..0a1f3b7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp)
> if (!error)
> error = security_path_truncate(path);
> if (!error) {
> - error = do_truncate(path->dentry, 0,
> + error = do_truncate(path, 0,
> ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
> filp);
> }
> diff --git a/fs/open.c b/fs/open.c
> index d6fd3ac..390e322 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -34,11 +34,12 @@
>
> #include "internal.h"
>
> -int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> +int do_truncate(struct path *path, loff_t length, unsigned int time_attrs,
> struct file *filp)
> {
> int ret;
> struct iattr newattrs;
> + struct dentry *dentry = path->dentry;
>
> /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
> if (length < 0)
> @@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>
> mutex_lock(&dentry->d_inode->i_mutex);
> /* Note any delegations or leases have already been broken: */
> - ret = notify_change(dentry, &newattrs, NULL);
> + ret = notify_change_path(path, &newattrs, NULL);
> mutex_unlock(&dentry->d_inode->i_mutex);
> return ret;
> }
> @@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length)
> if (!error)
> error = security_path_truncate(path);
> if (!error)
> - error = do_truncate(path->dentry, length, 0, NULL);
> + error = do_truncate(path, length, 0, NULL);
>
> put_write_and_out:
> put_write_access(inode);
> @@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
> if (!error)
> error = security_path_truncate(&f.file->f_path);
> if (!error)
> - error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
> + error = do_truncate(&f.file->f_path, length,
> + ATTR_MTIME|ATTR_CTIME, f.file);
> sb_end_write(inode->i_sb);
> out_putf:
> fdput(f);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4111ee0..e5b0b05 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2031,7 +2031,7 @@ struct filename {
> };
>
> extern long vfs_truncate(struct path *, loff_t);
> -extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
> +extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs,
> struct file *filp);
> extern int do_fallocate(struct file *file, int mode, loff_t offset,
> loff_t len);
> @@ -2253,6 +2253,7 @@ extern void emergency_remount(void);
> extern sector_t bmap(struct inode *, sector_t);
> #endif
> extern int notify_change(struct dentry *, struct iattr *, struct inode **);
> +extern int notify_change_path(struct path *, struct iattr *, struct inode **);
> extern int inode_permission(struct inode *, int);
> extern int generic_permission(struct inode *, int);
>
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 1c804b0..0fd594b 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry)
> * fsnotify_change - notify_change event. file was modified and/or metadata
> * was changed.
> */
> -static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
> +static inline void fsnotify_change(struct path *path, struct dentry *dentry,
> + unsigned int ia_valid)
> {
> struct inode *inode = dentry->d_inode;
> __u32 mask = 0;
> @@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
> if (S_ISDIR(inode->i_mode))
> mask |= FS_ISDIR;
>
> - fsnotify_parent(NULL, dentry, mask);
> - fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> + fsnotify_parent(path, dentry, mask);
> + if (path == NULL)
> + fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE,
> + NULL, 0);
> + else
> + fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH,
> + NULL, 0);
> }
> }
>
> --
> 2.1.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hello Andrew,
please, consider the patch in
https://lkml.org/lkml/2014/10/23/611
for inclusion in the MM tree.
It will ensure that FAN_MODIFY events are created for truncate() and
ftruncate().
To my knowledge these are the last two system calls changing files
directly that are not creating fanotify events.
As Jan Kara correctly points there a few other sources of changes to the
file system for which we still have to analyze if they should create
fanotify events.
E.g. meta-filesystems like ecryptfs and overlayfs trigger changes of the
underlying file systems.
Best regards
Heinrich Schuchardt
On 10.11.2014 21:30, Jan Kara wrote:
> On Thu 23-10-14 23:35:07, Heinrich Schuchardt wrote:
>> :: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote:
>> ::
>> :: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in
>> :: notify_change() is because we have only dentry available there. OTOH from a
>> :: quick look it doesn't look impossible to pass path there from the callers.
>> :: So I'd rather propagate path to notify_change() and generate also fanotify
>> :: events there than generating truncate events for fanotify separately
>> :: somewhere else...
>>
>> The attached second version of the patch follows this idea of Jan.
>>
>> The fanotify and fanotify API can be used to monitor changes of the file
>> system.
>>
>> System calls ftruncate and truncate modify files. Hence they should trigger
>> the corresponding fanotify (FAN_MODIFY) event.
>>
>> Prior to the patch only a inotify (IN_MODIFY) event is triggered because
>> the path information is not passed to the notification framework.
>>
>> The patch changes the interface of fsnotify_change to allow path information
>> to be passed. Fsnotify_change is only called by notify_change and by
>> fat_ioctl_set_attributes.
>>
>> notify_change is called by a larger number of callers. Not all of these
>> have access to path object. To limit the number of changes the patch renames
>> notify_change to do_notify_change and adds a path parameter. The patch further
>> adds two wrappers: notify_change for callers that cannot provide a path
>> and notify_change_path for callers that can provide a path.
>>
>> The patch changes do_truncate to accept a path parameter passed to
>> notify_change_path and adjusts all callers of do_truncate.
> So what I somewhat dislike about this patch is that notify_change() is
> sometimes called with dentry and sometimes with path. That way it's not
> completely clear when fanotify events will be generated and when not.
> Sadly it isn't easy to provide struct path in all the places where we are
> calling notify_change() so I'm not sure what would a better solution look
> like either :(
>
> Honza
>>
>> Signed-off-by: Heinrich Schuchardt <[email protected]>
>> ---
>> fs/attr.c | 35 ++++++++++++++++++++++++++++++++---
>> fs/fat/file.c | 2 +-
>> fs/namei.c | 2 +-
>> fs/open.c | 10 ++++++----
>> include/linux/fs.h | 3 ++-
>> include/linux/fsnotify.h | 12 +++++++++---
>> 6 files changed, 51 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/attr.c b/fs/attr.c
>> index 6530ced..894967e 100644
>> --- a/fs/attr.c
>> +++ b/fs/attr.c
>> @@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr)
>> EXPORT_SYMBOL(setattr_copy);
>>
>> /**
>> - * notify_change - modify attributes of a filesytem object
>> + * do_notify_change - modify attributes of a filesytem object
>> + * @path: path of object affected
>> * @dentry: object affected
>> * @iattr: new attributes
>> * @delegated_inode: returns inode, if the inode is delegated
>> *
>> * The caller must hold the i_mutex on the affected object.
>> *
>> + * If NULL is passed in attribute path, fanotify events cannot be
>> + * created.
>> + *
>> * If notify_change discovers a delegation in need of breaking,
>> * it will return -EWOULDBLOCK and return a reference to the inode in
>> * delegated_inode. The caller should then break the delegation and
>> @@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy);
>> * the file open for write, as there can be no conflicting delegation in
>> * that case.
>> */
>> -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode)
>> +int do_notify_change(struct path *path, struct dentry *dentry,
>> + struct iattr *attr, struct inode **delegated_inode)
>> {
>> struct inode *inode = dentry->d_inode;
>> umode_t mode = inode->i_mode;
>> @@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
>> error = simple_setattr(dentry, attr);
>>
>> if (!error) {
>> - fsnotify_change(dentry, ia_valid);
>> + fsnotify_change(path, dentry, ia_valid);
>> ima_inode_post_setattr(dentry);
>> evm_inode_post_setattr(dentry, ia_valid);
>> }
>>
>> return error;
>> }
>> +
>> +/*
>> + * notify_change - modify attributes of a filesytem object
>> + *
>> + * This call will not create fanotify events. It should be replaced by
>> + * notify_change_path where possible.
>> + */
>> +int notify_change(struct dentry *dentry, struct iattr *attr,
>> + struct inode **delegated_inode)
>> +{
>> + return do_notify_change(NULL, dentry, attr, delegated_inode);
>> +}
>> EXPORT_SYMBOL(notify_change);
>> +
>> +/*
>> + * notify_change_path - modify attributes of a filesytem object
>> + *
>> + * This call is a replacement for notify_change. It creates fanotify events.
>> + */
>> +int notify_change_path(struct path *path, struct iattr *attr,
>> + struct inode **delegated_inode)
>> +{
>> + return do_notify_change(path, path->dentry, attr, delegated_inode);
>> +}
>> +EXPORT_SYMBOL(notify_change_path);
>> diff --git a/fs/fat/file.c b/fs/fat/file.c
>> index f2c73ae..d293a0b 100644
>> --- a/fs/fat/file.c
>> +++ b/fs/fat/file.c
>> @@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr)
>> if (err)
>> goto out_unlock_inode;
>>
>> - fsnotify_change(file->f_path.dentry, ia.ia_valid);
>> + fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid);
>> if (sbi->options.sys_immutable) {
>> if (attr & ATTR_SYS)
>> inode->i_flags |= S_IMMUTABLE;
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 43927d1..0a1f3b7 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp)
>> if (!error)
>> error = security_path_truncate(path);
>> if (!error) {
>> - error = do_truncate(path->dentry, 0,
>> + error = do_truncate(path, 0,
>> ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
>> filp);
>> }
>> diff --git a/fs/open.c b/fs/open.c
>> index d6fd3ac..390e322 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -34,11 +34,12 @@
>>
>> #include "internal.h"
>>
>> -int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>> +int do_truncate(struct path *path, loff_t length, unsigned int time_attrs,
>> struct file *filp)
>> {
>> int ret;
>> struct iattr newattrs;
>> + struct dentry *dentry = path->dentry;
>>
>> /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
>> if (length < 0)
>> @@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>>
>> mutex_lock(&dentry->d_inode->i_mutex);
>> /* Note any delegations or leases have already been broken: */
>> - ret = notify_change(dentry, &newattrs, NULL);
>> + ret = notify_change_path(path, &newattrs, NULL);
>> mutex_unlock(&dentry->d_inode->i_mutex);
>> return ret;
>> }
>> @@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length)
>> if (!error)
>> error = security_path_truncate(path);
>> if (!error)
>> - error = do_truncate(path->dentry, length, 0, NULL);
>> + error = do_truncate(path, length, 0, NULL);
>>
>> put_write_and_out:
>> put_write_access(inode);
>> @@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
>> if (!error)
>> error = security_path_truncate(&f.file->f_path);
>> if (!error)
>> - error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file);
>> + error = do_truncate(&f.file->f_path, length,
>> + ATTR_MTIME|ATTR_CTIME, f.file);
>> sb_end_write(inode->i_sb);
>> out_putf:
>> fdput(f);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 4111ee0..e5b0b05 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -2031,7 +2031,7 @@ struct filename {
>> };
>>
>> extern long vfs_truncate(struct path *, loff_t);
>> -extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
>> +extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs,
>> struct file *filp);
>> extern int do_fallocate(struct file *file, int mode, loff_t offset,
>> loff_t len);
>> @@ -2253,6 +2253,7 @@ extern void emergency_remount(void);
>> extern sector_t bmap(struct inode *, sector_t);
>> #endif
>> extern int notify_change(struct dentry *, struct iattr *, struct inode **);
>> +extern int notify_change_path(struct path *, struct iattr *, struct inode **);
>> extern int inode_permission(struct inode *, int);
>> extern int generic_permission(struct inode *, int);
>>
>> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
>> index 1c804b0..0fd594b 100644
>> --- a/include/linux/fsnotify.h
>> +++ b/include/linux/fsnotify.h
>> @@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry)
>> * fsnotify_change - notify_change event. file was modified and/or metadata
>> * was changed.
>> */
>> -static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
>> +static inline void fsnotify_change(struct path *path, struct dentry *dentry,
>> + unsigned int ia_valid)
>> {
>> struct inode *inode = dentry->d_inode;
>> __u32 mask = 0;
>> @@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
>> if (S_ISDIR(inode->i_mode))
>> mask |= FS_ISDIR;
>>
>> - fsnotify_parent(NULL, dentry, mask);
>> - fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
>> + fsnotify_parent(path, dentry, mask);
>> + if (path == NULL)
>> + fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE,
>> + NULL, 0);
>> + else
>> + fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH,
>> + NULL, 0);
>> }
>> }
>>
>> --
>> 2.1.1
>>
On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote:
> So what I somewhat dislike about this patch is that notify_change() is
> sometimes called with dentry and sometimes with path. That way it's not
> completely clear when fanotify events will be generated and when not.
> Sadly it isn't easy to provide struct path in all the places where we are
> calling notify_change() so I'm not sure what would a better solution look
> like either :(
I suspect the right thing to do is to split out the truncate path
from notify_change, as it's fairly different anyway.
On Mon 10-11-14 23:34:15, Christoph Hellwig wrote:
> On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote:
> > So what I somewhat dislike about this patch is that notify_change() is
> > sometimes called with dentry and sometimes with path. That way it's not
> > completely clear when fanotify events will be generated and when not.
> > Sadly it isn't easy to provide struct path in all the places where we are
> > calling notify_change() so I'm not sure what would a better solution look
> > like either :(
>
> I suspect the right thing to do is to split out the truncate path
> from notify_change, as it's fairly different anyway.
Yeah, that would make sense. I wanted to say it's quite a lot of work to
change all the filesystems (where the separation of truncate path makes
sense as well IMHO) but actually it's possible to just do the separation at
the VFS level and still call ->setattr() fs callback for now. Heinrich will
you look into this?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 11.11.2014 12:09, Jan Kara wrote:
> On Mon 10-11-14 23:34:15, Christoph Hellwig wrote:
>> On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote:
>>> So what I somewhat dislike about this patch is that notify_change() is
>>> sometimes called with dentry and sometimes with path. That way it's not
>>> completely clear when fanotify events will be generated and when not.
No. With my patch notify_change is still only called with dentry.
It is only new function notify_change_path which will be called with a path.
And this function will only be called from do_truncate up to now.
>>> Sadly it isn't easy to provide struct path in all the places where we are
>>> calling notify_change() so I'm not sure what would a better solution look
>>> like either :(
We only want to create FAN_MODIFY events for ATTR_SIZE. So only for
these events we need a path.
To my knowledge notify_change is called with ATTR_SIZE from
do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE
from ovl_setattr() for a truncation.
ecryptfs_dentry_to_lower_path() could be used in ecryptfs_truncate() to
obtain a path.
ovl_path_upper() could be used in ovl_setattr() to obtain a path.
>>
>> I suspect the right thing to do is to split out the truncate path
>> from notify_change, as it's fairly different anyway.
> Yeah, that would make sense. I wanted to say it's quite a lot of work to
> change all the filesystems (where the separation of truncate path makes
> sense as well IMHO) but actually it's possible to just do the separation at
> the VFS level and still call ->setattr() fs callback for now. Heinrich will
> you look into this?
You seem to agree that struct path has to passed to do_truncate() and
further to the notification layer.
Currently do_truncate() calls notify_change() which does not accept a
path argument.
Here the size change is implemented as an attribute change.
Furthermore time attributes are changed.
What do you exactly mean by
> just do the separation at the virtual file system level
> and still call ->setattr() fs callback for now.
Do you want to duplicate the logic of notify_change() to a
new function notify_truncate() which will at least have to handle
ATTR_SIZE, ATTR_FORCE, ATTR_KILL_SUID, ATTR_KILL_SGID, ATTR_FILE and
time attributes?
And from notify_truncate() call new function fsnotify_truncate() with a
logic like fsnotify_modify() but accepting a path?
This would result in a lot of code duplication.
In which respect would such a patch be preferable?
Best regards
Heinrich Schuchardt
On Tue 11-11-14 20:55:26, Heinrich Schuchardt wrote:
> On 11.11.2014 12:09, Jan Kara wrote:
> >On Mon 10-11-14 23:34:15, Christoph Hellwig wrote:
> >>On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote:
> >>> So what I somewhat dislike about this patch is that notify_change() is
> >>>sometimes called with dentry and sometimes with path. That way it's not
> >>>completely clear when fanotify events will be generated and when not.
>
> No. With my patch notify_change is still only called with dentry.
> It is only new function notify_change_path which will be called with a path.
> And this function will only be called from do_truncate up to now.
>
> >>>Sadly it isn't easy to provide struct path in all the places where we are
> >>>calling notify_change() so I'm not sure what would a better solution look
> >>>like either :(
>
> We only want to create FAN_MODIFY events for ATTR_SIZE. So only for
> these events we need a path.
>
> To my knowledge notify_change is called with ATTR_SIZE from
> do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE
> from ovl_setattr() for a truncation.
There's also a call in fs/cachefiles/interface.c: cachefiles_attr_changed()
and fs/hpfs/namei.c: hpfs_unlink() and nfs plays with ATTR_SIZE although I
wasn't able to track down whether it actually passes it to notify_change().
So I agree you have covered almost all the cases. But it's just prone to
errors (as you can see when you missed some cases) because it isn't clear
to callers of notify_change() whether and why they should provide the path
via notify_change_path() or not. But see below.
> ecryptfs_dentry_to_lower_path() could be used in ecryptfs_truncate()
> to obtain a path.
> ovl_path_upper() could be used in ovl_setattr() to obtain a path.
Agreed, this can and should be done.
> >>I suspect the right thing to do is to split out the truncate path
> >>from notify_change, as it's fairly different anyway.
> > Yeah, that would make sense. I wanted to say it's quite a lot of work to
> >change all the filesystems (where the separation of truncate path makes
> >sense as well IMHO) but actually it's possible to just do the separation at
> >the VFS level and still call ->setattr() fs callback for now. Heinrich will
> >you look into this?
>
> You seem to agree that struct path has to passed to do_truncate()
> and further to the notification layer.
Yes.
> Currently do_truncate() calls notify_change() which does not accept
> a path argument.
>
> Here the size change is implemented as an attribute change.
> Furthermore time attributes are changed.
>
> What do you exactly mean by
> > just do the separation at the virtual file system level
> > and still call ->setattr() fs callback for now.
>
> Do you want to duplicate the logic of notify_change() to a
> new function notify_truncate() which will at least have to handle
> ATTR_SIZE, ATTR_FORCE, ATTR_KILL_SUID, ATTR_KILL_SGID, ATTR_FILE and
> time attributes?
>
> And from notify_truncate() call new function fsnotify_truncate()
> with a logic like fsnotify_modify() but accepting a path?
>
> This would result in a lot of code duplication.
Yes, I forgot about other changes coupled with truncate and you're right
that duplicating them would do more bad than good. But what we could do
is to just have __notify_change() which takes both path & dentry as you did
it. notify_change() which takes just dentry and verifies ATTR_SIZE is not
set (WARN_ON_ONCE if it is). notify_change_truncate() which takes path.
This still isn't pretty but at least makes errors harder to do.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hello Jan,
thank you for the review.
On 14.11.2014 11:01, Jan Kara wrote:
>>
>> We only want to create FAN_MODIFY events for ATTR_SIZE. So only for
>> these events we need a path.
>>
>> To my knowledge notify_change is called with ATTR_SIZE from
>> do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE
>> from ovl_setattr() for a truncation.
> There's also a call in fs/cachefiles/interface.c: cachefiles_attr_changed()
> and fs/hpfs/namei.c: hpfs_unlink() and nfs plays with ATTR_SIZE although I
> wasn't able to track down whether it actually passes it to notify_change().
cachefiles_attr_changed() contains two calls to notify_change(). Both
calls pass ATTR_SIZE as sole attribute.
In cachefiles_attr_changed() we can create a path object with
path.dentry = object->backer;
path.mnt = cache->mnt;
In hpfs_unlink() a truncation occurs only if deletion fails.
I do not see how to access a path here.
notify_change() is not called in directory fs/nfs.
grep -GHrn notify_change fs/nfs/
This does not test for any indirect call.
Best regards
Heinrich Schuchardt