On 02-02-2023 12:13, Luca Vizzarro wrote:

The interface for fcntl expects the argument passed for the command F_SETLEASE to be of type int. The current code wrongly treats it as a long.

This commit changes the signature of all the related and helper functions so that they treat the argument as int instead of long.

Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com

fs/cifs/cifsfs.c | 2 +- fs/fcntl.c | 3 ++- fs/libfs.c | 2 +- fs/locks.c | 22 +++++++++++----------- fs/nfs/nfs4file.c | 2 +- fs/nfs/nfs4proc.c | 4 ++-- include/linux/fs.h | 16 ++++++++-------- 7 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index 712a43161448..7e519b6a3f6b 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1065,7 +1065,7 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence) } static int -cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **priv) +cifs_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { /* * Note that this is called by vfs setlease with i_lock held to diff --git a/fs/fcntl.c b/fs/fcntl.c index 918d0136d12b..081ec2ae82b9 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -316,6 +316,7 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t arg, struct file *filp) { void __user *argp = (void __user *)arg;

• int argi = (int)argi;

One too many argi on this line 🙂

This reminds me, make sure to run the LTP fcntl test suite to verify your changes.

Thanks, Tudor

struct flock flock; long err = -EINVAL; @@ -400,7 +401,7 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t arg, err = fcntl_getlease(filp); break; case F_SETLEASE:

• err = fcntl_setlease(fd, filp, arg);
  

• err = fcntl_setlease(fd, filp, argi);
  

  break; case F_NOTIFY: err = fcntl_dirnotify(fd, filp, arg);

diff --git a/fs/libfs.c b/fs/libfs.c index 682d56345a1c..ee47a22605c0 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1258,7 +1258,7 @@ EXPORT_SYMBOL(alloc_anon_inode);

• All arguments are ignored and it just returns -EINVAL.

*/ int -simple_nosetlease(struct file *filp, long arg, struct file_lock **flp, +simple_nosetlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { return -EINVAL; diff --git a/fs/locks.c b/fs/locks.c index 607f94a0e789..7405b02db496 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -437,7 +437,7 @@ static void flock_make_lock(struct file *filp, struct file_lock *fl, int type) fl->fl_end = OFFSET_MAX; } -static int assign_type(struct file_lock *fl, long type) +static int assign_type(struct file_lock *fl, int type) { switch (type) { case F_RDLCK: @@ -548,7 +548,7 @@ static const struct lock_manager_operations lease_manager_ops = { /*

• Initialize a lease, use the default lock manager operations

*/ -static int lease_init(struct file *filp, long type, struct file_lock *fl) +static int lease_init(struct file *filp, int type, struct file_lock *fl) { if (assign_type(fl, type) != 0) return -EINVAL; @@ -566,7 +566,7 @@ static int lease_init(struct file *filp, long type, struct file_lock *fl) } /* Allocate a file_lock initialised to this type of lease */ -static struct file_lock *lease_alloc(struct file *filp, long type) +static struct file_lock *lease_alloc(struct file *filp, int type) { struct file_lock *fl = locks_alloc_lock(); int error = -ENOMEM; @@ -1665,7 +1665,7 @@ int fcntl_getlease(struct file *filp)

• conflict with the lease we're trying to set.

*/ static int -check_conflicting_open(struct file *filp, const long arg, int flags) +check_conflicting_open(struct file *filp, const int arg, int flags) { struct inode *inode = locks_inode(filp); int self_wcount = 0, self_rcount = 0; @@ -1700,7 +1700,7 @@ check_conflicting_open(struct file *filp, const long arg, int flags) } static int -generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **priv) +generic_add_lease(struct file *filp, int arg, struct file_lock **flp, void **priv) { struct file_lock *fl, *my_fl = NULL, *lease; struct inode *inode = locks_inode(filp); @@ -1858,7 +1858,7 @@ static int generic_delete_lease(struct file *filp, void *owner)

• The (input) flp->fl_lmops->lm_break function is required
• by break_lease().

*/ -int generic_setlease(struct file *filp, long arg, struct file_lock **flp, +int generic_setlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { struct inode *inode = locks_inode(filp); @@ -1905,7 +1905,7 @@ lease_notifier_chain_init(void) } static inline void -setlease_notifier(long arg, struct file_lock *lease) +setlease_notifier(int arg, struct file_lock *lease) { if (arg != F_UNLCK) srcu_notifier_call_chain(&lease_notifier_chain, arg, lease); @@ -1930,7 +1930,7 @@ lease_notifier_chain_init(void) } static inline void -setlease_notifier(long arg, struct file_lock *lease) +setlease_notifier(int arg, struct file_lock *lease) { } @@ -1965,7 +1965,7 @@ EXPORT_SYMBOL_GPL(lease_unregister_notifier);

• may be NULL if the lm_setup operation doesn't require it.

*/ int -vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) +vfs_setlease(struct file *filp, int arg, struct file_lock **lease, void **priv) { if (lease) setlease_notifier(arg, *lease); @@ -1976,7 +1976,7 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **priv) } EXPORT_SYMBOL_GPL(vfs_setlease); -static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) +static int do_fcntl_add_lease(unsigned int fd, struct file *filp, int arg) { struct file_lock *fl; struct fasync_struct *new; @@ -2011,7 +2011,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)

• Note that you also need to call %F_SETSIG to
• receive a signal when the lease is broken.

*/ -int fcntl_setlease(unsigned int fd, struct file *filp, long arg) +int fcntl_setlease(unsigned int fd, struct file *filp, int arg) { if (arg == F_UNLCK) return vfs_setlease(filp, F_UNLCK, NULL, (void **)&filp); diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 9eb181287879..1bc7a05e7e7e 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -442,7 +442,7 @@ void nfs42_ssc_unregister_ops(void) } #endif /* CONFIG_NFS_V4_2 */ -static int nfs4_setlease(struct file *file, long arg, struct file_lock **lease, +static int nfs4_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { return nfs4_proc_setlease(file, arg, lease, priv); diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 86ed5c0142c3..d7529f2761bb 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -7553,7 +7553,7 @@ static int nfs4_delete_lease(struct file *file, void **priv) return generic_setlease(file, F_UNLCK, NULL, priv); } -static int nfs4_add_lease(struct file *file, long arg, struct file_lock **lease, +static int nfs4_add_lease(struct file *file, int arg, struct file_lock **lease, void **priv) { struct inode *inode = file_inode(file); @@ -7571,7 +7571,7 @@ static int nfs4_add_lease(struct file *file, long arg, struct file_lock **lease, return -EAGAIN; } -int nfs4_proc_setlease(struct file *file, long arg, struct file_lock **lease, +int nfs4_proc_setlease(struct file *file, int arg, struct file_lock **lease, void **priv) { switch (arg) { diff --git a/include/linux/fs.h b/include/linux/fs.h index ded8c97516f3..d939351461fe 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1151,7 +1151,7 @@ extern int fcntl_setlk64(unsigned int, struct file *, unsigned int, struct flock64 *); #endif -extern int fcntl_setlease(unsigned int fd, struct file *filp, long arg); +extern int fcntl_setlease(unsigned int fd, struct file *filp, int arg); extern int fcntl_getlease(struct file *filp); /* fs/locks.c */ @@ -1173,8 +1173,8 @@ extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl); extern int locks_lock_inode_wait(struct inode *inode, struct file_lock *fl); extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type); extern void lease_get_mtime(struct inode *, struct timespec64 *time); -extern int generic_setlease(struct file *, long, struct file_lock **, void **priv); -extern int vfs_setlease(struct file *, long, struct file_lock **, void **); +extern int generic_setlease(struct file *, int, struct file_lock **, void **priv); +extern int vfs_setlease(struct file *, int, struct file_lock **, void **); extern int lease_modify(struct file_lock *, int, struct list_head *); struct notifier_block; @@ -1212,7 +1212,7 @@ static inline int fcntl_setlk64(unsigned int fd, struct file *file, return -EACCES; } #endif -static inline int fcntl_setlease(unsigned int fd, struct file *filp, long arg) +static inline int fcntl_setlease(unsigned int fd, struct file *filp, int arg) { return -EINVAL; } @@ -1300,13 +1300,13 @@ static inline void lease_get_mtime(struct inode *inode, return; } -static inline int generic_setlease(struct file *filp, long arg, +static inline int generic_setlease(struct file *filp, int arg, struct file_lock **flp, void **priv) { return -EINVAL; } -static inline int vfs_setlease(struct file *filp, long arg, +static inline int vfs_setlease(struct file *filp, int arg, struct file_lock **lease, void **priv) { return -EINVAL; @@ -2131,7 +2131,7 @@ struct file_operations { int (*flock) (struct file *, int, struct file_lock *); ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int); ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int);

• int (*setlease)(struct file *, long, struct file_lock **, void **);

• int (*setlease)(struct file *, int, struct file_lock **, void **); long (*fallocate)(struct file *file, int mode, loff_t offset, loff_t len); void (*show_fdinfo)(struct seq_file *m, struct file *f);

@@ -3350,7 +3350,7 @@ extern int simple_write_begin(struct file *file, struct address_space *mapping, extern const struct address_space_operations ram_aops; extern int always_delete_dentry(const struct dentry *); extern struct inode *alloc_anon_inode(struct super_block *); -extern int simple_nosetlease(struct file *, long, struct file_lock **, void **); +extern int simple_nosetlease(struct file *, int, struct file_lock **, void **); extern const struct dentry_operations simple_dentry_operations; extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);

On 02-02-2023 13:36, Tudor Cretu wrote:

On 02-02-2023 12:13, Luca Vizzarro wrote:

...

The interface for fcntl expects the argument passed for the command F_SETPIPE_SZ to be of type int. The current code wrongly treats it as a long.

This commit changes the signature of all the related and helper functions so that they treat the argument as int instead of long.

Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com

fs/fcntl.c                | 2 +-   fs/pipe.c                 | 6 +++---   include/linux/pipe_fs_i.h | 4 ++--   3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c index db9f03ab23d3..dc416037d9fc 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -408,7 +408,7 @@ static long do_fcntl(int fd, unsigned int cmd, user_uintptr_t arg,           break;       case F_SETPIPE_SZ:       case F_GETPIPE_SZ: -        err = pipe_fcntl(filp, cmd, arg); +        err = pipe_fcntl(filp, cmd, argi);           break;       case F_ADD_SEALS:       case F_GET_SEALS: diff --git a/fs/pipe.c b/fs/pipe.c index 76169e370e64..79f3eb118f11 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -1231,7 +1231,7 @@ const struct file_operations pipefifo_fops = {    * Currently we rely on the pipe array holding a power-of-2 number    * of pages. Returns 0 on error.    */ -unsigned int round_pipe_size(unsigned long size) +unsigned int round_pipe_size(unsigned int size)   {       if (size > (1U << 31))           return 0; @@ -1314,7 +1314,7 @@ int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots)    * Allocate a new array of pipe buffers and copy the info over. Returns the    * pipe size if successful, or return -ERROR on error.    */ -static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) +static long pipe_set_size(struct pipe_inode_info *pipe, unsigned int arg)   {       unsigned long user_bufs;       unsigned int nr_slots, size; @@ -1382,7 +1382,7 @@ struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice)       return pipe;   } -long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg) +long pipe_fcntl(struct file *file, unsigned int cmd, unsigned int arg)

Hi Luca,

Please, correct me if I'm wrong, but this function seems to be called for both F_SETPIPE_SZ and F_GETPIPE_SZ. The issue is the type of arg expected for F_GETPIPE_SZ is void (no argument). So in the case of F_GETPIPE_SZ, the kernel accesses undefined bytes even with this change. My first idea is to separate the handler into two, one for F_SETPIPE_SZ and one for F_GETPIPE_SZ. I haven't looked in detail to see if that would cause issues, so feel free to explore the issue and other options as well.

Thanks, Tudor

On a second thought, I don't think this would cause any issue because, as you mentioned, the F_GETPIPE_SZ doesn't use arg. While I still prefer separate handlers given that one uses arg, and the other doesn't, I think it's fine to leave it like this as well. So feel free to ignore this comment, as well as the one for F_{ADD,GET}_SEALS. Maybe someone else can chime in on this with a stronger argument if they see fit.

Thanks, Tudor

...

{       struct pipe_inode_info *pipe;       long ret; diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h index 6cb65df3e3ba..49da593edfed 100644 --- a/include/linux/pipe_fs_i.h +++ b/include/linux/pipe_fs_i.h @@ -253,10 +253,10 @@ bool pipe_is_unprivileged_user(void);   #ifdef CONFIG_WATCH_QUEUE   int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots);   #endif -long pipe_fcntl(struct file *, unsigned int, unsigned long arg); +long pipe_fcntl(struct file *, unsigned int, unsigned int arg);   struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);   int create_pipe_files(struct file **, int); -unsigned int round_pipe_size(unsigned long size); +unsigned int round_pipe_size(unsigned int size);   #endif

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org