getting rid of int *open in ->atomic_open() and friends
Subsystems (21)
Al Viro
maintainerRFC PATCH 1/1 getting rid of int *open in ->atomic_open() and friends
->atomic_open() calling conventions are nasty; we have
two bits of state ("has file gotten past ->open()" and "have we created a new file here") passed by reference, with rather brittle logics in the callers. In some error cases do_last() et.al. end up doing a full-blow fput() (and destroying the struct file they've got), in some they leave that work (a trimmed-down subset of it, at that) to path_openat(), with the aforementioned state affecting what's going on.
It would be much easier if we stored the "has the
damn thing been past ->open()" (i.e. does it need fput() or put_filp()) in file->f_mode. It doesn't take a lot of massage to do that - mostly it's a matter of leaving all struct file freeing in error cases to path_openat() itself and preventing the wipeout of all ->f_mode bits in do_dentry_open(). Moreover, we can store the "has it been created" in the same ->f_mode, killing the 'int *opened' mess completely. The same series gets O_DIRECT checks properly centralized in do_dentry_open(), BTW.
Other fun stuff possible (but not done yet) is
unification of put_filp() and fput() and hopefully more simple rules regarding those. Need to sort out some things about pipe and ia64 perfmon first, though...
It's still a work-in-progress; what I've got right
now is in vfs.git#work.open;
current shortlog:
Al Viro (13):
drm_mode_create_lease_ioctl(): switch to filp_clone_open() rename filp_clone_open() to file_clone_open()
introduce FMODE_OPENED
get rid of 'opened' argument of finish_open()
pull fput() on late failures into path_openat()
switch all remaining checks for FILE_OPENED to FMODE_OPENED
now we can fold open_check_o_direct() into do_dentry_open() __gfs2_lookup(), nfs_finish_open() and fuse_create_open() don't need 'opened' introduce FMODE_CREATED and switch to it
IMA: don't propagate opened through the entire thing gfs2_create_inode() doesn't need 'opened' anymore
get rid of 'opened' argument of ->atomic_open()
get rid of 'opened' in path_openat() and the helpers downstream
diffstat:
arch/ia64/kernel/perfmon.c | 1 + drivers/gpu/drm/drm_lease.c | 16 +------------- drivers/misc/cxl/api.c | 2 +- drivers/staging/lustre/lustre/llite/namei.c | 11 +++++----- fs/9p/vfs_inode.c | 7 +++--- fs/9p/vfs_inode_dotl.c | 7 +++--- fs/aio.c | 3 ++- fs/anon_inodes.c | 2 +- fs/bad_inode.c | 2 +- fs/binfmt_misc.c | 2 +- fs/ceph/file.c | 7 +++--- fs/ceph/super.h | 3 +-- fs/cifs/cifsfs.h | 3 +-- fs/cifs/dir.c | 7 +++--- fs/fuse/dir.c | 10 ++++----- fs/gfs2/inode.c | 32 +++++++++++++-------------- fs/hugetlbfs/inode.c | 2 +- fs/internal.h | 2 -- fs/namei.c | 82 ++++++++++++++++++++++++++++------------------------------------------ fs/nfs/dir.c | 14 ++++++------ fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4proc.c | 2 +- fs/nfsd/vfs.c | 2 +- fs/open.c | 48 +++++++++++++++-------------------------- fs/pipe.c | 2 ++ include/linux/fs.h | 10 ++++++--- include/linux/ima.h | 4 ++-- ipc/shm.c | 2 +- mm/shmem.c | 2 +- net/socket.c | 2 +- security/integrity/ima/ima.h | 4 ++-- security/integrity/ima/ima_appraise.c | 4 ++-- security/integrity/ima/ima_main.c | 16 +++++++------- 33 files changed, 135 insertions(+), 180 deletions(-)
and it's been only slightly build-tested. Review and comments would be welcome.
Al Viro
maintainerPATCH 1/13 drm_mode_create_lease_ioctl(): switch to filp_clone_open()
From: Al Viro <[email protected]>
It had been open-coding that and, AFAICS, getting the error case wrong.
--- drivers/gpu/drm/drm_lease.c | 16 +--------------- fs/internal.h | 1 - include/linux/fs.h | 1 + 3 files changed, 2 insertions(+), 16 deletions(-)
drivers/gpu/drm/drm_lease.c +1 -15
@@ -553,24 +553,13 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,/* Clone the lessor file to create a new file for us */ DRM_DEBUG_LEASE ("Allocating lease file\n" ); -path_get (&lessor_file->f_path); - lessee_file =alloc_file (&lessor_file->f_path, - lessor_file->f_mode, -fops_get (lessor_file->f_inode->i_fop)); - + lessee_file =filp_clone_open (lessor_file);if (IS_ERR (lessee_file)) { ret =PTR_ERR (lessee_file); goto out_lessee; } -/* Initialize the new file for DRM */ -DRM_DEBUG_LEASE ("Initializing the file with %p\n" , lessee_file->f_op->open); - ret = lessee_file->f_op->open (lessee_file->f_inode, lessee_file); -if (ret) - goto out_lessee_file; - lessee_priv = lessee_file->private_data; -/* Change the file to a master one */ drm_master_put (&lessee_priv->master); lessee_priv->master = lessee;
@@ -588,9 +577,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,DRM_DEBUG_LEASE ("drm_mode_create_lease_ioctl succeeded\n" );return 0 ; -out_lessee_file: -fput (lessee_file); - out_lessee:drm_master_put (&lessee);
fs/internal.h +0 -1
@@ -127,7 +127,6 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,extern int open_check_o_direct (struct file *f);extern int vfs_open (const struct path *,struct file *,const struct cred *); -extern struct file *filp_clone_open (struct file *);/* * inode.c
include/linux/fs.h +1 -0
@@ -2401,6 +2401,7 @@ extern struct file *filp_open(const char *, int, umode_t);extern struct file *file_open_root (struct dentry *,struct vfsmount *,const char *,int ,umode_t );extern struct file *dentry_open (const struct path *,int ,const struct cred *); +extern struct file *filp_clone_open (struct file *);extern int filp_close (struct file *,fl_owner_t id);extern struct filename *getname_flags (const char __user *,int ,int *);
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/13 rename filp_clone_open() to file_clone_open()
From: Al Viro <[email protected]>
'filp' naming convention is bogus; sure, it's "file pointer", but we generally don't do that kind of Hungarian notation. Some of the instances have too many callers to touch, but this one has only two, so let's sanitize it while we can...
--- drivers/gpu/drm/drm_lease.c | 2 +- fs/binfmt_misc.c | 2 +- fs/open.c | 4 ++-- include/linux/fs.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_lease.c +1 -1
@@ -553,7 +553,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,/* Clone the lessor file to create a new file for us */ DRM_DEBUG_LEASE ("Allocating lease file\n" ); - lessee_file =filp_clone_open (lessor_file); + lessee_file =file_clone_open (lessor_file);if (IS_ERR (lessee_file)) { ret =PTR_ERR (lessee_file); goto out_lessee;
fs/binfmt_misc.c +1 -1
@@ -205,7 +205,7 @@ static int load_misc_binary(struct linux_binprm *bprm) goto error;if (fmt->flags &MISC_FMT_OPEN_FILE ) { - interp_file =filp_clone_open (fmt->interp_file); + interp_file =file_clone_open (fmt->interp_file);if (!IS_ERR (interp_file))deny_write_access (interp_file); }else {
fs/open.c +2 -2
@@ -1063,7 +1063,7 @@ struct file *file_open_root(struct dentry *dentry, struct vfsmount *mnt, }EXPORT_SYMBOL (file_open_root); -struct file *filp_clone_open (struct file *oldfile) +struct file *file_clone_open (struct file *oldfile) {struct file *file;int retval;
@@ -1081,7 +1081,7 @@ struct file *filp_clone_open(struct file *oldfile)return file; } -EXPORT_SYMBOL (filp_clone_open); +EXPORT_SYMBOL (file_clone_open);long do_sys_open (int dfd,const char __user *filename,int flags,umode_t mode) {
include/linux/fs.h +1 -1
@@ -2401,7 +2401,7 @@ extern struct file *filp_open(const char *, int, umode_t);extern struct file *file_open_root (struct dentry *,struct vfsmount *,const char *,int ,umode_t );extern struct file *dentry_open (const struct path *,int ,const struct cred *); -extern struct file *filp_clone_open (struct file *); +extern struct file *file_clone_open (struct file *);extern int filp_close (struct file *,fl_owner_t id);extern struct filename *getname_flags (const char __user *,int ,int *);
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/13 introduce FMODE_OPENED
In reply to Al ViroFrom: Al Viro <[email protected]>
basically, "is that instance set up enough for regular fput(), or do we want put_filp() for that one".
--- arch/ia64/kernel/perfmon.c | 1 + drivers/misc/cxl/api.c | 2 +- fs/aio.c | 3 ++- fs/anon_inodes.c | 2 +- fs/hugetlbfs/inode.c | 2 +- fs/open.c | 3 ++- fs/pipe.c | 2 ++ include/linux/fs.h | 3 +++ ipc/shm.c | 2 +- mm/shmem.c | 2 +- net/socket.c | 2 +- 11 files changed, 16 insertions(+), 8 deletions(-)
arch/ia64/kernel/perfmon.c +1 -0
@@ -2684,6 +2684,7 @@ pfm_context_create(pfm_context_t *ctx, void *arg, int count, struct pt_regs *reg* initialize soft PMU state */ pfm_reset_pmu_state (ctx); + filp->f_mode |=FMODE_OPENED ;fd_install (fd, filp);
drivers/misc/cxl/api.c +1 -1
@@ -102,7 +102,7 @@ static struct file *cxl_getfile(const char *name, path.mnt =mntget (cxl_vfs_mount);d_instantiate (path.dentry, inode); - file =alloc_file (&path,OPEN_FMODE (flags), fops); + file =alloc_file (&path,OPEN_FMODE (flags) |FMODE_OPENED , fops);if (IS_ERR (file)) goto err_dput; file->f_flags = flags & (O_ACCMODE |O_NONBLOCK );
fs/aio.c +2 -1
@@ -223,7 +223,8 @@ static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages) path.mnt =mntget (aio_mnt);d_instantiate (path.dentry, inode); - file =alloc_file (&path,FMODE_READ |FMODE_WRITE , &aio_ring_fops); + file =alloc_file (&path,FMODE_READ |FMODE_WRITE |FMODE_OPENED , + &aio_ring_fops);if (IS_ERR (file)) {path_put (&path);return file;
fs/anon_inodes.c +1 -1
@@ -102,7 +102,7 @@ struct file *anon_inode_getfile(const char *name,d_instantiate (path.dentry, anon_inode_inode); - file =alloc_file (&path,OPEN_FMODE (flags), fops); + file =alloc_file (&path,OPEN_FMODE (flags) |FMODE_OPENED , fops);if (IS_ERR (file)) goto err_dput; file->f_mapping = anon_inode_inode->i_mapping;
fs/hugetlbfs/inode.c +1 -1
@@ -1375,7 +1375,7 @@ struct file *hugetlb_file_setup(const char *name, size_t size, inode->i_size = size;clear_nlink (inode); - file =alloc_file (&path,FMODE_WRITE |FMODE_READ , + file =alloc_file (&path,FMODE_WRITE |FMODE_READ |FMODE_OPENED , &hugetlbfs_file_operations);if (IS_ERR (file)) goto out_dentry;/* inode is already attached */
fs/open.c +2 -1
@@ -753,7 +753,7 @@ static int do_dentry_open(struct file *f, f->f_wb_err =filemap_sample_wb_err (f->f_mapping);if (unlikely (f->f_flags &O_PATH )) { - f->f_mode =FMODE_PATH ; + f->f_mode =FMODE_PATH |FMODE_OPENED ; f->f_op = &empty_fops;return 0 ; }
@@ -795,6 +795,7 @@ static int do_dentry_open(struct file *f,if (error) goto cleanup_all; } + f->f_mode |=FMODE_OPENED ;if ((f->f_mode & (FMODE_READ |FMODE_WRITE )) ==FMODE_READ )i_readcount_inc (inode); if ((f->f_mode & FMODE_READ) &&
fs/pipe.c +2 -0
@@ -772,6 +772,8 @@ int create_pipe_files(struct file **res, int flags) goto err_file; } + res[0 ]->f_mode |=FMODE_OPENED ; + f->f_mode |=FMODE_OPENED ;path_get (&path); res[0 ]->private_data = inode->i_pipe; res[0 ]->f_flags =O_RDONLY | (flags &O_NONBLOCK );
include/linux/fs.h +3 -0
@@ -147,12 +147,15 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,/* Has write method(s) */ #define FMODE_CAN_WRITE ((__force fmode_t)0x40000) +#define FMODE_OPENED ((__force fmode_t)0x80000) +/* File was opened by fanotify and shouldn't generate fanotify events */ #define FMODE_NONOTIFY ((__force fmode_t)0x4000000)/* File is capable of returning -EAGAIN if I/O will block */ #define FMODE_NOWAIT ((__force fmode_t)0x8000000) +/* * Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector * that indicates that they should check the contents of the iovec are
ipc/shm.c +1 -1
@@ -1438,7 +1438,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, goto out_nattch; } - file =alloc_file (&path, f_mode, + file =alloc_file (&path, f_mode |FMODE_OPENED ,is_file_hugepages (shp->shm_file) ? &shm_file_operations_huge : &shm_file_operations);
mm/shmem.c +1 -1
@@ -4239,7 +4239,7 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name, lif (IS_ERR (res)) goto put_path; - res =alloc_file (&path,FMODE_WRITE |FMODE_READ , + res =alloc_file (&path,FMODE_WRITE |FMODE_READ |FMODE_OPENED , &shmem_file_operations);if (IS_ERR (res)) goto put_path;
net/socket.c +1 -1
@@ -407,7 +407,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)d_instantiate (path.dentry,SOCK_INODE (sock)); - file =alloc_file (&path,FMODE_READ |FMODE_WRITE , + file =alloc_file (&path,FMODE_READ |FMODE_WRITE |FMODE_OPENED , &socket_file_ops);if (IS_ERR (file)) {/* drop dentry, keep inode for a bit */
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 4/13 get rid of 'opened' argument of finish_open()
In reply to Al ViroFrom: Al Viro <[email protected]>
We only deal with opened & FILE_OPENED in it and it's parallel to file->f_mode & FMODE_OPENED. So let the callers deal with the latter instead. Most of the call chains go through fs/namei.c:atomic_open() and do not look at FILE_OPENED between the return from finish_open() and return to atomic_open(), which reduces the size of patch nicely - we propagate to *opened in atomic_open() and that leaves us with do_tmpfile() (where we do the same) and gfs2_atomic_open() (where we check ->f_mode instead of *opened).
Signed-off-by Al Viro <[email protected]>
--- drivers/staging/lustre/lustre/llite/namei.c | 2 +- fs/9p/vfs_inode.c | 2 +- fs/9p/vfs_inode_dotl.c | 2 +- fs/ceph/file.c | 2 +- fs/cifs/dir.c | 2 +- fs/fuse/dir.c | 2 +- fs/gfs2/inode.c | 8 ++++---- fs/namei.c | 6 +++++- fs/nfs/dir.c | 2 +- fs/open.c | 14 ++++---------- include/linux/fs.h | 3 +-- 11 files changed, 21 insertions(+), 24 deletions(-)
drivers/staging/lustre/lustre/llite/namei.c +1 -1
@@ -748,7 +748,7 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry, rc =finish_no_open (file, de); }else { file->private_data = it; - rc =finish_open (file, dentry,NULL , opened); + rc =finish_open (file, dentry,NULL );/* We dget in ll_splice_alias. finish_open takes * care of dget for fd open. */
fs/9p/vfs_inode.c +1 -1
@@ -922,7 +922,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, v9inode->writeback_fid = (void *) inode_fid; }mutex_unlock (&v9inode->v_mutex); - err =finish_open (file, dentry, generic_file_open, opened); + err =finish_open (file, dentry, generic_file_open);if (err) goto error;
fs/9p/vfs_inode_dotl.c +1 -1
@@ -352,7 +352,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, }mutex_unlock (&v9inode->v_mutex);/* Since we are opening a file, assign the open fid to the file */ - err =finish_open (file, dentry, generic_file_open, opened); + err =finish_open (file, dentry, generic_file_open);if (err) goto err_clunk_old_fid; file->private_data = ofid;
fs/ceph/file.c +1 -1
@@ -474,7 +474,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,ceph_init_inode_acls (d_inode (dentry), &acls); *opened |=FILE_CREATED ; } - err =finish_open (file, dentry, ceph_open, opened); + err =finish_open (file, dentry, ceph_open); } out_req:if (!req->r_err && req->r_target_inode)
fs/cifs/dir.c +1 -1
@@ -541,7 +541,7 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,if ((oflags & (O_CREAT |O_EXCL )) == (O_CREAT |O_EXCL )) *opened |=FILE_CREATED ; - rc =finish_open (file, direntry, generic_file_open, opened); + rc =finish_open (file, direntry, generic_file_open);if (rc) {if (server->ops->close) server->ops->close (xid, tcon, &fid);
fs/fuse/dir.c +1 -1
@@ -469,7 +469,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,d_instantiate (entry, inode);fuse_change_entry_timeout (entry, &outentry);fuse_invalidate_attr (dir); - err =finish_open (file, entry, generic_file_open, opened); + err =finish_open (file, entry, generic_file_open);if (err) {fuse_sync_release (ff, flags); }else {
fs/gfs2/inode.c +4 -4
@@ -626,7 +626,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, error =0 ;if (file) {if (S_ISREG (inode->i_mode)) - error =finish_open (file, dentry, gfs2_open_common, opened); + error =finish_open (file, dentry, gfs2_open_common);else error =finish_no_open (file,NULL ); }
@@ -768,7 +768,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,d_instantiate (dentry, inode);if (file) { *opened |=FILE_CREATED ; - error =finish_open (file, dentry, gfs2_open_common, opened); + error =finish_open (file, dentry, gfs2_open_common); }gfs2_glock_dq_uninit (ghs);gfs2_glock_dq_uninit (ghs +1 );
@@ -866,7 +866,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,return d; }if (file &&S_ISREG (inode->i_mode)) - error =finish_open (file, dentry, gfs2_open_common, opened); + error =finish_open (file, dentry, gfs2_open_common);gfs2_glock_dq_uninit (&gh);if (error) {
@@ -1250,7 +1250,7 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,if (d !=NULL ) dentry = d;if (d_really_is_positive (dentry)) { -if (!(*opened &FILE_OPENED )) +if (!(file->f_mode &FMODE_OPENED ))return finish_no_open (file, d);dput (d);return 0 ;
fs/namei.c +5 -1
@@ -3027,6 +3027,8 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,* permission here. */ int acc_mode = op->acc_mode; +if (file->f_mode &FMODE_OPENED ) + *opened |=FILE_OPENED ;if (*opened &FILE_CREATED ) {WARN_ON (!(open_flag &O_CREAT ));fsnotify_create (dir, dentry);
@@ -3446,9 +3448,11 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,if (error) goto out2; file->f_path.mnt = path.mnt; - error =finish_open (file, child,NULL , opened); + error =finish_open (file, child,NULL );if (error) goto out2; +if (file->f_mode &FMODE_OPENED ) + *opened |=FILE_OPENED ; error =open_check_o_direct (file);if (error)fput (file);
fs/nfs/dir.c +1 -1
@@ -1423,7 +1423,7 @@ static int nfs_finish_open(struct nfs_open_context *ctx, {int err; - err =finish_open (file, dentry, do_open, opened); + err =finish_open (file, dentry, do_open);if (err) goto out;if (S_ISREG (file->f_path.dentry->d_inode->i_mode))
fs/open.c +4 -10
@@ -742,7 +742,7 @@ static int do_dentry_open(struct file *f,static const struct file_operations empty_fops = {};int error; - f->f_mode =OPEN_FMODE (f->f_flags) |FMODE_LSEEK | + f->f_mode |=OPEN_FMODE (f->f_flags) |FMODE_LSEEK |FMODE_PREAD |FMODE_PWRITE ;path_get (&f->f_path);
@@ -848,19 +848,13 @@ static int do_dentry_open(struct file *f,* Returns zero on success or -errno if the open failed. */ int finish_open (struct file *file,struct dentry *dentry, -int (*open)(struct inode *,struct file *), -int *opened) +int (*open)(struct inode *,struct file *)) { -int error; -BUG_ON (*opened &FILE_OPENED );/* once it's opened, it's opened */ +BUG_ON (file->f_mode &FMODE_OPENED );/* once it's opened, it's opened */ file->f_path.dentry = dentry; - error =do_dentry_open (file,d_backing_inode (dentry), open, +return do_dentry_open (file,d_backing_inode (dentry), open,current_cred ()); -if (!error) - *opened |=FILE_OPENED ; - -return error; }EXPORT_SYMBOL (finish_open);
include/linux/fs.h +1 -2
@@ -2417,8 +2417,7 @@ enum { FILE_OPENED = 2 };extern int finish_open (struct file *file,struct dentry *dentry, -int (*open)(struct inode *,struct file *), -int *opened); +int (*open)(struct inode *,struct file *));extern int finish_no_open (struct file *file,struct dentry *dentry);/* fs/ioctl.c */
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 5/13 pull fput() on late failures into path_openat()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/namei.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
fs/namei.c +15 -16
@@ -3375,8 +3375,6 @@ static int do_last(struct nameidata *nd,if (!error && will_truncate) error =handle_truncate (file); out: -if (unlikely (error) && (*opened &FILE_OPENED )) -fput (file);if (unlikely (error >0 )) {WARN_ON (1 ); error = -EINVAL ;
@@ -3454,8 +3452,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,if (file->f_mode &FMODE_OPENED ) *opened |=FILE_OPENED ; error =open_check_o_direct (file); -if (error) -fput (file); out2:mnt_drop_write (path.mnt); out:
@@ -3517,20 +3513,23 @@ static struct file *path_openat(struct nameidata *nd, }terminate_walk (nd); out2: -if (!(opened &FILE_OPENED )) { -BUG_ON (!error); -put_filp (file); +if (likely (!error)) { +if (likely (opened &FILE_OPENED )) +return file; +WARN_ON (1 ); + error = -EINVAL ; } -if (unlikely (error)) { -if (error == -EOPENSTALE ) { -if (flags &LOOKUP_RCU ) - error = -ECHILD ; -else - error = -ESTALE ; - } - file =ERR_PTR (error); +if (opened &FILE_OPENED ) +fput (file); +else +put_filp (file); +if (error == -EOPENSTALE ) { +if (flags &LOOKUP_RCU ) + error = -ECHILD ; +else + error = -ESTALE ; } -return file; +return ERR_PTR (error); } struct file *do_filp_open(int dfd, struct filename *pathname,
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 6/13 switch all remaining checks for FILE_OPENED to FMODE_OPENED
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/namei.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
fs/namei.c +3 -10
@@ -3027,8 +3027,6 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,* permission here. */ int acc_mode = op->acc_mode; -if (file->f_mode &FMODE_OPENED ) - *opened |=FILE_OPENED ;if (*opened &FILE_CREATED ) {WARN_ON (!(open_flag &O_CREAT ));fsnotify_create (dir, dentry);
@@ -3363,11 +3361,10 @@ static int do_last(struct nameidata *nd, error =may_open (&nd->path, acc_mode, open_flag);if (error) goto out; -BUG_ON (*opened &FILE_OPENED );/* once it's opened, it's opened */ +BUG_ON (file->f_mode &FMODE_OPENED );/* once it's opened, it's opened */ error =vfs_open (&nd->path, file,current_cred ());if (error) goto out; - *opened |=FILE_OPENED ; opened: error =open_check_o_direct (file);if (!error)
@@ -3449,8 +3446,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, error =finish_open (file, child,NULL );if (error) goto out2; -if (file->f_mode &FMODE_OPENED ) - *opened |=FILE_OPENED ; error =open_check_o_direct (file); out2:mnt_drop_write (path.mnt);
@@ -3492,8 +3487,6 @@ static struct file *path_openat(struct nameidata *nd,if (unlikely (file->f_flags &O_PATH )) { error =do_o_path (nd, flags, file); -if (!error) - opened |=FILE_OPENED ; goto out2; }
@@ -3514,12 +3507,12 @@ static struct file *path_openat(struct nameidata *nd,terminate_walk (nd); out2:if (likely (!error)) { -if (likely (opened &FILE_OPENED )) +if (likely (file->f_mode &FMODE_OPENED ))return file;WARN_ON (1 ); error = -EINVAL ; } -if (opened &FILE_OPENED ) +if (file->f_mode &FMODE_OPENED )fput (file);else put_filp (file);
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 7/13 now we can fold open_check_o_direct() into do_dentry_open()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/internal.h | 1 - fs/namei.c | 7 +------ fs/open.c | 27 +++++++++------------------ 3 files changed, 10 insertions(+), 25 deletions(-)
fs/internal.h +0 -1
@@ -125,7 +125,6 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode);int do_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag); -extern int open_check_o_direct (struct file *f);extern int vfs_open (const struct path *,struct file *,const struct cred *);/*
fs/namei.c +1 -6
@@ -3366,9 +3366,7 @@ static int do_last(struct nameidata *nd,if (error) goto out; opened: - error =open_check_o_direct (file); -if (!error) - error =ima_file_check (file, op->acc_mode, *opened); + error =ima_file_check (file, op->acc_mode, *opened);if (!error && will_truncate) error =handle_truncate (file); out:
@@ -3444,9 +3442,6 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags, goto out2; file->f_path.mnt = path.mnt; error =finish_open (file, child,NULL ); -if (error) - goto out2; - error =open_check_o_direct (file); out2:mnt_drop_write (path.mnt); out:
fs/open.c +9 -18
@@ -724,16 +724,6 @@ SYSCALL_DEFINE3(fchown, unsigned int, fd, uid_t, user, gid_t, group)return ksys_fchown (fd, user, group); } -int open_check_o_direct (struct file *f) -{ -/* NB: we're sure to have correct a_ops only after f_op->open */ -if (f->f_flags &O_DIRECT ) { -if (!f->f_mapping->a_ops || !f->f_mapping->a_ops->direct_IO) -return -EINVAL ; - } -return 0 ; -} - static int do_dentry_open(struct file *f, struct inode *inode, int (*open)(struct inode *, struct file *),
@@ -810,6 +800,11 @@ static int do_dentry_open(struct file *f,file_ra_state_init (&f->f_ra, f->f_mapping->host->i_mapping); +/* NB: we're sure to have correct a_ops only after f_op->open */ +if (f->f_flags &O_DIRECT ) { +if (!f->f_mapping->a_ops || !f->f_mapping->a_ops->direct_IO) +return -EINVAL ; + }return 0 ; cleanup_all:
@@ -918,15 +913,11 @@ struct file *dentry_open(const struct path *path, int flags,if (!IS_ERR (f)) { f->f_flags = flags; error =vfs_open (path, f, cred); -if (!error) { -/* from now on we need fput() to dispose of f */ - error =open_check_o_direct (f); -if (error) { +if (error) { +if (f->f_mode &FMODE_OPENED )fput (f); - f =ERR_PTR (error); - } - }else { -put_filp (f); +else +put_filp (f); f =ERR_PTR (error); } }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 8/13 __gfs2_lookup(), nfs_finish_open() and fuse_create_open() don't need 'opened'
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/fuse/dir.c | 4 ++-- fs/gfs2/inode.c | 7 +++---- fs/nfs/dir.c | 5 ++--- 3 files changed, 7 insertions(+), 9 deletions(-)
fs/fuse/dir.c +2 -2
@@ -399,7 +399,7 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,*/ static int fuse_create_open (struct inode *dir,struct dentry *entry,struct file *file,unsigned flags, -umode_t mode,int *opened) +umode_t mode) {int err;struct inode *inode;
@@ -513,7 +513,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,if (fc->no_create) goto mknod; - err =fuse_create_open (dir, entry, file, flags, mode, opened); + err =fuse_create_open (dir, entry, file, flags, mode);if (err == -ENOSYS ) { fc->no_create =1 ; goto mknod;
fs/gfs2/inode.c +3 -4
@@ -830,14 +830,13 @@ static int gfs2_create(struct inode *dir, struct dentry *dentry,* @dir: The directory inode * @dentry: The dentry of the new inode * @file: File to be opened -* @opened: atomic_open flags * * * Returns: errno */ static struct dentry *__gfs2_lookup (struct inode *dir,struct dentry *dentry, -struct file *file,int *opened) +struct file *file) {struct inode *inode;struct dentry *d;
@@ -879,7 +878,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,static struct dentry *gfs2_lookup (struct inode *dir,struct dentry *dentry,unsigned flags) { -return __gfs2_lookup (dir, dentry,NULL ,NULL ); +return __gfs2_lookup (dir, dentry,NULL ); }/**
@@ -1244,7 +1243,7 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,if (!d_in_lookup (dentry)) goto skip_lookup; - d =__gfs2_lookup (dir, dentry, file, opened); + d =__gfs2_lookup (dir, dentry, file);if (IS_ERR (d))return PTR_ERR (d);if (d !=NULL )
fs/nfs/dir.c +2 -3
@@ -1418,8 +1418,7 @@ static int do_open(struct inode *inode, struct file *filp)static int nfs_finish_open (struct nfs_open_context *ctx,struct dentry *dentry, -struct file *file,unsigned open_flags, -int *opened) +struct file *file,unsigned open_flags) {int err;
@@ -1530,7 +1529,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, goto out; } - err =nfs_finish_open (ctx, ctx->dentry, file, open_flags, opened); + err =nfs_finish_open (ctx, ctx->dentry, file, open_flags);trace_nfs_atomic_open_exit (dir, ctx, open_flags, err);put_nfs_open_context (ctx); out:
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 9/13 introduce FMODE_CREATED and switch to it
In reply to Al ViroFrom: Al Viro <[email protected]>
Parallel to FILE_CREATED, goes into ->f_mode instead of *opened. NFS is a bit of a wart here - it doesn't have file at the point where FILE_CREATED used to be set, so we need to propagate it there (for now). IMA is another one (here and everywhere)...
--- drivers/staging/lustre/lustre/llite/namei.c | 2 +- fs/9p/vfs_inode.c | 2 +- fs/9p/vfs_inode_dotl.c | 2 +- fs/ceph/file.c | 2 +- fs/cifs/dir.c | 2 +- fs/fuse/dir.c | 2 +- fs/gfs2/inode.c | 2 +- fs/namei.c | 15 ++++++++------- fs/nfs/dir.c | 5 ++++- fs/nfs/nfs4proc.c | 2 +- include/linux/fs.h | 1 + 11 files changed, 21 insertions(+), 16 deletions(-)
drivers/staging/lustre/lustre/llite/namei.c +1 -1
@@ -735,7 +735,7 @@ static int ll_atomic_open(struct inode *dir, struct dentry *dentry, goto out_release; } - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; }if (d_really_is_positive (dentry) &&it_disposition (it,DISP_OPEN_OPEN )) {
fs/9p/vfs_inode.c +1 -1
@@ -930,7 +930,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,if (v9ses->cache ==CACHE_LOOSE || v9ses->cache ==CACHE_FSCACHE )v9fs_cache_inode_set_cookie (d_inode (dentry), file); - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; out:dput (res);return err;
fs/9p/vfs_inode_dotl.c +1 -1
@@ -358,7 +358,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry, file->private_data = ofid;if (v9ses->cache ==CACHE_LOOSE || v9ses->cache ==CACHE_FSCACHE )v9fs_cache_inode_set_cookie (inode, file); - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; out:v9fs_put_acl (dacl, pacl);dput (res);
fs/ceph/file.c +1 -1
@@ -472,7 +472,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,dout ("atomic_open finish_open on dn %p\n" , dn);if (req->r_op ==CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {ceph_init_inode_acls (d_inode (dentry), &acls); - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; } err =finish_open (file, dentry, ceph_open); }
fs/cifs/dir.c +1 -1
@@ -539,7 +539,7 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry, }if ((oflags & (O_CREAT |O_EXCL )) == (O_CREAT |O_EXCL )) - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; rc =finish_open (file, direntry, generic_file_open);if (rc) {
fs/fuse/dir.c +1 -1
@@ -508,7 +508,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry, goto no_open;/* Only creates */ - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ;if (fc->no_create) goto mknod;
fs/gfs2/inode.c +1 -1
@@ -767,7 +767,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,mark_inode_dirty (inode);d_instantiate (dentry, inode);if (file) { - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ; error =finish_open (file, dentry, gfs2_open_common); }gfs2_glock_dq_uninit (ghs);
fs/namei.c +8 -7
@@ -3027,7 +3027,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,* permission here. */ int acc_mode = op->acc_mode; -if (*opened &FILE_CREATED ) { +if (file->f_mode &FMODE_CREATED ) {WARN_ON (!(open_flag &O_CREAT ));fsnotify_create (dir, dentry); acc_mode =0 ;
@@ -3043,7 +3043,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,dput (dentry); dentry = file->f_path.dentry; } -if (*opened &FILE_CREATED ) +if (file->f_mode &FMODE_CREATED )fsnotify_create (dir, dentry);if (unlikely (d_is_negative (dentry))) { error = -ENOENT ;
@@ -3092,7 +3092,7 @@ static int lookup_open(struct nameidata *nd, struct path *path,if (unlikely (IS_DEADDIR (dir_inode)))return -ENOENT ; - *opened &= ~FILE_CREATED ; + file->f_mode &= ~FMODE_CREATED ; dentry =d_lookup (dir, &nd->last);for (;;) {if (!dentry) {
@@ -3177,7 +3177,7 @@ static int lookup_open(struct nameidata *nd, struct path *path,/* Negative dentry, just create the file */ if (!dentry->d_inode && (open_flag &O_CREAT )) { - *opened |=FILE_CREATED ; + file->f_mode |=FMODE_CREATED ;audit_inode_child (dir_inode, dentry,AUDIT_TYPE_CHILD_CREATE );if (!dir_inode->i_op->create) { error = -EACCES ;
@@ -3284,7 +3284,7 @@ static int do_last(struct nameidata *nd,if (error) goto out; -if ((*opened &FILE_CREATED ) || +if ((file->f_mode &FMODE_CREATED ) || !S_ISREG (file_inode (file)->i_mode)) will_truncate = false;
@@ -3292,7 +3292,7 @@ static int do_last(struct nameidata *nd, goto opened; } -if (*opened &FILE_CREATED ) { +if (file->f_mode &FMODE_CREATED ) {/* Don't check for write permission, don't truncate */ open_flag &= ~O_TRUNC ; will_truncate = false;
@@ -3366,7 +3366,8 @@ static int do_last(struct nameidata *nd,if (error) goto out; opened: - error =ima_file_check (file, op->acc_mode, *opened); + error =ima_file_check (file, op->acc_mode, + file->f_mode &FMODE_CREATED ?FILE_CREATED :0 );if (!error && will_truncate) error =handle_truncate (file); out:
fs/nfs/dir.c +4 -1
@@ -1444,6 +1444,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,struct inode *inode;unsigned int lookup_flags =0 ;bool switched = false; +int created =0 ;int err;/* Expect a negative dentry */
@@ -1504,7 +1505,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry, goto out;trace_nfs_atomic_open_enter (dir, ctx, open_flags); - inode =NFS_PROTO (dir)->open_context (dir, ctx, open_flags, &attr, opened); + inode =NFS_PROTO (dir)->open_context (dir, ctx, open_flags, &attr, &created); +if (created) + file->f_mode |=FMODE_CREATED ;if (IS_ERR (inode)) { err =PTR_ERR (inode);trace_nfs_atomic_open_exit (dir, ctx, open_flags, err);
fs/nfs/nfs4proc.c +1 -1
@@ -2934,7 +2934,7 @@ static int _nfs4_do_open(struct inode *dir, } }if (opened && opendata->file_created) - *opened |=FILE_CREATED ; + *opened =1 ;if (pnfs_use_threshold (ctx_th, opendata->f_attr.mdsthreshold, server)) { *ctx_th = opendata->f_attr.mdsthreshold;
include/linux/fs.h +1 -0
@@ -148,6 +148,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,#define FMODE_CAN_WRITE ((__force fmode_t)0x40000)#define FMODE_OPENED ((__force fmode_t)0x80000) +#define FMODE_CREATED ((__force fmode_t)0x100000)/* File was opened by fanotify and shouldn't generate fanotify events */ #define FMODE_NONOTIFY ((__force fmode_t)0x4000000)
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 10/13 IMA: don't propagate opened through the entire thing
In reply to Al ViroFrom: Al Viro <[email protected]>
just check ->f_mode in ima_appraise_measurement()
--- fs/namei.c | 3 +-- fs/nfsd/vfs.c | 2 +- include/linux/ima.h | 4 ++-- security/integrity/ima/ima.h | 4 ++-- security/integrity/ima/ima_appraise.c | 4 ++-- security/integrity/ima/ima_main.c | 16 ++++++++-------- 6 files changed, 16 insertions(+), 17 deletions(-)
fs/namei.c +1 -2
@@ -3366,8 +3366,7 @@ static int do_last(struct nameidata *nd,if (error) goto out; opened: - error =ima_file_check (file, op->acc_mode, - file->f_mode &FMODE_CREATED ?FILE_CREATED :0 ); + error =ima_file_check (file, op->acc_mode);if (!error && will_truncate) error =handle_truncate (file); out:
fs/nfsd/vfs.c +1 -1
@@ -763,7 +763,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, goto out_nfserr; } - host_err =ima_file_check (file, may_flags,0 ); + host_err =ima_file_check (file, may_flags);if (host_err) {fput (file); goto out_nfserr;
include/linux/ima.h +2 -2
@@ -16,7 +16,7 @@ struct linux_binprm;#ifdef CONFIG_IMA extern int ima_bprm_check (struct linux_binprm *bprm); -extern int ima_file_check (struct file *file,int mask,int opened); +extern int ima_file_check (struct file *file,int mask);extern void ima_file_free (struct file *file);extern int ima_file_mmap (struct file *file,unsigned long prot);extern int ima_read_file (struct file *file,enum kernel_read_file_id id);
@@ -34,7 +34,7 @@ static inline int ima_bprm_check(struct linux_binprm *bprm)return 0 ; } -static inline int ima_file_check (struct file *file,int mask,int opened) +static inline int ima_file_check (struct file *file,int mask) {return 0 ; }
security/integrity/ima/ima.h +2 -2
@@ -239,7 +239,7 @@ int ima_appraise_measurement(enum ima_hooks func,struct integrity_iint_cache *iint, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, - int xattr_len, int opened); + int xattr_len);int ima_must_appraise (struct inode *inode,int mask,enum ima_hooks func);void ima_update_xattr (struct integrity_iint_cache *iint,struct file *file); enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
@@ -255,7 +255,7 @@ static inline int ima_appraise_measurement(enum ima_hooks func,struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, - int xattr_len, int opened) + int xattr_len) { return INTEGRITY_UNKNOWN; }
security/integrity/ima/ima_appraise.c +2 -2
@@ -212,7 +212,7 @@ int ima_appraise_measurement(enum ima_hooks func,struct integrity_iint_cache *iint, struct file *file, const unsigned char *filename, struct evm_ima_xattr_data *xattr_value, - int xattr_len, int opened) + int xattr_len) { static const char op[] = "appraise_data";const char *cause ="unknown" ;
@@ -231,7 +231,7 @@ int ima_appraise_measurement(enum ima_hooks func, cause = iint->flags &IMA_DIGSIG_REQUIRED ?"IMA-signature-required" :"missing-hash" ; status =INTEGRITY_NOLABEL ; -if (opened &FILE_CREATED ) +if (file->f_mode &FMODE_CREATED ) iint->flags |=IMA_NEW_FILE ; if ((iint->flags & IMA_NEW_FILE) && (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
security/integrity/ima/ima_main.c +8 -8
@@ -173,7 +173,7 @@ void ima_file_free(struct file *file)static int process_measurement (struct file *file,const struct cred *cred,u32 secid,char *buf,loff_t size,int mask, -enum ima_hooks func,int opened) +enum ima_hooks func) {struct inode *inode =file_inode (file);struct integrity_iint_cache *iint =NULL ;
@@ -299,7 +299,7 @@ static int process_measurement(struct file *file, const struct cred *cred,if (rc ==0 && (action &IMA_APPRAISE_SUBMASK )) {inode_lock (inode); rc =ima_appraise_measurement (func, iint, file, pathname, - xattr_value, xattr_len, opened); + xattr_value, xattr_len);inode_unlock (inode); }if (action &IMA_AUDIT )
@@ -343,7 +343,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)if (file && (prot &PROT_EXEC )) {security_task_getsecid (current, &secid);return process_measurement (file,current_cred (), secid,NULL , -0 ,MAY_EXEC ,MMAP_CHECK ,0 ); +0 ,MAY_EXEC ,MMAP_CHECK ); }return 0 ;
@@ -369,13 +369,13 @@ int ima_bprm_check(struct linux_binprm *bprm)security_task_getsecid (current, &secid); ret =process_measurement (bprm->file,current_cred (), secid,NULL ,0 , -MAY_EXEC ,BPRM_CHECK ,0 ); +MAY_EXEC ,BPRM_CHECK );if (ret)return ret;security_cred_getsecid (bprm->cred, &secid);return process_measurement (bprm->file, bprm->cred, secid,NULL ,0 , -MAY_EXEC ,CREDS_CHECK ,0 ); +MAY_EXEC ,CREDS_CHECK ); }/**
@@ -388,14 +388,14 @@ int ima_bprm_check(struct linux_binprm *bprm)* On success return 0. On integrity appraisal error, assuming the file * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. */ -int ima_file_check (struct file *file,int mask,int opened) +int ima_file_check (struct file *file,int mask) {u32 secid;security_task_getsecid (current, &secid);return process_measurement (file,current_cred (), secid,NULL ,0 , mask & (MAY_READ |MAY_WRITE |MAY_EXEC | -MAY_APPEND ),FILE_CHECK , opened); +MAY_APPEND ),FILE_CHECK ); }EXPORT_SYMBOL_GPL (ima_file_check);
@@ -497,7 +497,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, func = read_idmap[read_id] ?:FILE_CHECK ;security_task_getsecid (current, &secid);return process_measurement (file,current_cred (), secid, buf, size, -MAY_READ , func,0 ); +MAY_READ , func); }static int __init init_ima (void )
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 11/13 gfs2_create_inode() doesn't need 'opened' anymore
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/gfs2/inode.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
fs/gfs2/inode.c +6 -6
@@ -580,7 +580,7 @@ static int gfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array,static int gfs2_create_inode (struct inode *dir,struct dentry *dentry,struct file *file,umode_t mode,dev_t dev,const char *symname, -unsigned int size,int excl,int *opened) +unsigned int size,int excl) {const struct qstr *name = &dentry->d_name;struct posix_acl *default_acl, *acl;
@@ -822,7 +822,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,static int gfs2_create (struct inode *dir,struct dentry *dentry,umode_t mode,bool excl) { -return gfs2_create_inode (dir, dentry,NULL ,S_IFREG | mode,0 ,NULL ,0 , excl,NULL ); +return gfs2_create_inode (dir, dentry,NULL ,S_IFREG | mode,0 ,NULL ,0 , excl); }/**
@@ -1188,7 +1188,7 @@ static int gfs2_symlink(struct inode *dir, struct dentry *dentry,if (size >=gfs2_max_stuffed_size (GFS2_I (dir)))return -ENAMETOOLONG ; -return gfs2_create_inode (dir, dentry,NULL ,S_IFLNK |S_IRWXUGO ,0 , symname, size,0 ,NULL ); +return gfs2_create_inode (dir, dentry,NULL ,S_IFLNK |S_IRWXUGO ,0 , symname, size,0 ); }/**
@@ -1203,7 +1203,7 @@ static int gfs2_symlink(struct inode *dir, struct dentry *dentry,static int gfs2_mkdir (struct inode *dir,struct dentry *dentry,umode_t mode) {unsigned dsize =gfs2_max_stuffed_size (GFS2_I (dir)); -return gfs2_create_inode (dir, dentry,NULL ,S_IFDIR | mode,0 ,NULL , dsize,0 ,NULL ); +return gfs2_create_inode (dir, dentry,NULL ,S_IFDIR | mode,0 ,NULL , dsize,0 ); }/**
@@ -1218,7 +1218,7 @@ static int gfs2_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)static int gfs2_mknod (struct inode *dir,struct dentry *dentry,umode_t mode,dev_t dev) { -return gfs2_create_inode (dir, dentry,NULL , mode, dev,NULL ,0 ,0 ,NULL ); +return gfs2_create_inode (dir, dentry,NULL , mode, dev,NULL ,0 ,0 ); }/**
@@ -1261,7 +1261,7 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,if (!(flags &O_CREAT ))return -ENOENT ; -return gfs2_create_inode (dir, dentry, file,S_IFREG | mode,0 ,NULL ,0 , excl, opened); +return gfs2_create_inode (dir, dentry, file,S_IFREG | mode,0 ,NULL ,0 , excl); }/*
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 12/13 get rid of 'opened' argument of ->atomic_open()
In reply to Al ViroFrom: Al Viro <[email protected]>
not used anymore
--- drivers/staging/lustre/lustre/llite/namei.c | 7 +++---- fs/9p/vfs_inode.c | 3 +-- fs/9p/vfs_inode_dotl.c | 3 +-- fs/bad_inode.c | 2 +- fs/ceph/file.c | 3 +-- fs/ceph/super.h | 3 +-- fs/cifs/cifsfs.h | 3 +-- fs/cifs/dir.c | 3 +-- fs/fuse/dir.c | 2 +- fs/gfs2/inode.c | 3 +-- fs/namei.c | 3 +-- fs/nfs/dir.c | 2 +- fs/nfs/nfs4_fs.h | 2 +- include/linux/fs.h | 2 +- 14 files changed, 16 insertions(+), 25 deletions(-)
drivers/staging/lustre/lustre/llite/namei.c +3 -4
@@ -678,15 +678,14 @@ static struct dentry *ll_lookup_nd(struct inode *parent, struct dentry *dentry,*/ static int ll_atomic_open (struct inode *dir,struct dentry *dentry,struct file *file,unsigned int open_flags, -umode_t mode,int *opened) +umode_t mode) {struct lookup_intent *it;struct dentry *de;int rc =0 ; -CDEBUG (D_VFSTRACE ,"VFS Op:name=%pd, dir=" DFID "(%p),file %p,open_flags %x,mode %x opened %d\n" , - dentry,PFID (ll_inode2fid (dir)), dir, file, open_flags, mode, - *opened); +CDEBUG (D_VFSTRACE ,"VFS Op:name=%pd, dir=" DFID "(%p),file %p,open_flags %x,mode %x\n" , + dentry,PFID (ll_inode2fid (dir)), dir, file, open_flags, mode);/* Only negative dentries enter here */ LASSERT (!d_inode (dentry));
fs/9p/vfs_inode.c +1 -2
@@ -864,8 +864,7 @@ struct dentry *v9fs_vfs_lookup(struct inode *dir, struct dentry *dentry,static int v9fs_vfs_atomic_open (struct inode *dir,struct dentry *dentry, -struct file *file,unsigned flags,umode_t mode, -int *opened) +struct file *file,unsigned flags,umode_t mode) {int err;u32 perm;
fs/9p/vfs_inode_dotl.c +1 -2
@@ -241,8 +241,7 @@ v9fs_vfs_create_dotl(struct inode *dir, struct dentry *dentry, umode_t omode,static int v9fs_vfs_atomic_open_dotl (struct inode *dir,struct dentry *dentry, -struct file *file,unsigned flags,umode_t omode, -int *opened) +struct file *file,unsigned flags,umode_t omode) {int err =0 ;kgid_t gid;
fs/bad_inode.c +1 -1
@@ -134,7 +134,7 @@ static int bad_inode_update_time(struct inode *inode, struct timespec *time,static int bad_inode_atomic_open (struct inode *inode,struct dentry *dentry,struct file *file,unsigned int open_flag, -umode_t create_mode,int *opened) +umode_t create_mode) {return -EIO ; }
fs/ceph/file.c +1 -2
@@ -394,8 +394,7 @@ int ceph_open(struct inode *inode, struct file *file)* file or symlink, return 1 so the VFS can retry. */ int ceph_atomic_open (struct inode *dir,struct dentry *dentry, -struct file *file,unsigned flags,umode_t mode, -int *opened) +struct file *file,unsigned flags,umode_t mode) {struct ceph_fs_client *fsc =ceph_sb_to_client (dir->i_sb);struct ceph_mds_client *mdsc = fsc->mdsc;
fs/ceph/super.h +1 -2
@@ -1025,8 +1025,7 @@ extern const struct file_operations ceph_file_fops;extern int ceph_renew_caps (struct inode *inode);extern int ceph_open (struct inode *inode,struct file *file);extern int ceph_atomic_open (struct inode *dir,struct dentry *dentry, -struct file *file,unsigned flags,umode_t mode, -int *opened); +struct file *file,unsigned flags,umode_t mode);extern int ceph_release (struct inode *inode,struct file *filp);extern void ceph_fill_inline_data (struct inode *inode,struct page *locked_page,char *data,size_t len);
fs/cifs/cifsfs.h +1 -2
@@ -65,8 +65,7 @@ extern struct inode *cifs_root_iget(struct super_block *);extern int cifs_create (struct inode *,struct dentry *,umode_t ,bool excl);extern int cifs_atomic_open (struct inode *,struct dentry *, -struct file *,unsigned ,umode_t , -int *); +struct file *,unsigned ,umode_t );extern struct dentry *cifs_lookup (struct inode *,struct dentry *,unsigned int );extern int cifs_unlink (struct inode *dir,struct dentry *dentry);
fs/cifs/dir.c +1 -2
@@ -465,8 +465,7 @@ cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned int xid,int cifs_atomic_open (struct inode *inode,struct dentry *direntry, -struct file *file,unsigned oflags,umode_t mode, -int *opened) +struct file *file,unsigned oflags,umode_t mode) {int rc;unsigned int xid;
fs/fuse/dir.c +1 -1
@@ -489,7 +489,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,static int fuse_mknod (struct inode *,struct dentry *,umode_t ,dev_t );static int fuse_atomic_open (struct inode *dir,struct dentry *entry,struct file *file,unsigned flags, -umode_t mode,int *opened) +umode_t mode) {int err;struct fuse_conn *fc =get_fuse_conn (dir);
fs/gfs2/inode.c +1 -2
@@ -1228,14 +1228,13 @@ static int gfs2_mknod(struct inode *dir, struct dentry *dentry, umode_t mode,* @file: The proposed new struct file * @flags: open flags * @mode: File mode -* @opened: Flag to say whether the file has been opened or not * * Returns: error code or 0 for success */ static int gfs2_atomic_open (struct inode *dir,struct dentry *dentry,struct file *file,unsigned flags, -umode_t mode,int *opened) +umode_t mode) {struct dentry *d;bool excl = !!(flags &O_EXCL );
fs/namei.c +1 -2
@@ -3018,8 +3018,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry, file->f_path.dentry =DENTRY_NOT_SET ; file->f_path.mnt = nd->path.mnt; error = dir->i_op->atomic_open (dir, dentry, file, -open_to_namei_flags (open_flag), - mode, opened); +open_to_namei_flags (open_flag), mode);d_lookup_done (dentry);if (!error) {/*
fs/nfs/dir.c +1 -1
@@ -1435,7 +1435,7 @@ static int nfs_finish_open(struct nfs_open_context *ctx,int nfs_atomic_open (struct inode *dir,struct dentry *dentry,struct file *file,unsigned open_flags, -umode_t mode,int *opened) +umode_t mode) {DECLARE_WAIT_QUEUE_HEAD_ONSTACK (wq);struct nfs_open_context *ctx;
fs/nfs/nfs4_fs.h +1 -1
@@ -233,7 +233,7 @@ extern const struct dentry_operations nfs4_dentry_operations;/* dir.c */ int nfs_atomic_open (struct inode *,struct dentry *,struct file *, -unsigned ,umode_t ,int *); +unsigned ,umode_t );/* super.c */ extern struct file_system_type nfs4_fs_type;
include/linux/fs.h +1 -1
@@ -1771,7 +1771,7 @@ struct inode_operations {int (*update_time)(struct inode *,struct timespec *,int );int (*atomic_open)(struct inode *,struct dentry *,struct file *,unsigned open_flag, -umode_t create_mode,int *opened); +umode_t create_mode);int (*tmpfile) (struct inode *,struct dentry *,umode_t );int (*set_acl)(struct inode *,struct posix_acl *,int ); }____cacheline_aligned ;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 13/13 get rid of 'opened' in path_openat() and the helpers downstream
In reply to Al ViroFrom: Al Viro <[email protected]>
unused now
--- fs/namei.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
fs/namei.c +8 -14
@@ -3002,8 +3002,7 @@ static int may_o_create(const struct path *dir, struct dentry *dentry, umode_t mstatic int atomic_open (struct nameidata *nd,struct dentry *dentry,struct path *path,struct file *file,const struct open_flags *op, -int open_flag,umode_t mode, -int *opened) +int open_flag,umode_t mode) {struct dentry *const DENTRY_NOT_SET = (void *)-1UL ;struct inode *dir = nd->path.dentry->d_inode;
@@ -3071,14 +3070,11 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,* specified then a negative dentry may be returned. * * An error code is returned otherwise. -* -* FILE_CREATE will be set in @*opened if the dentry was created and will be -* cleared otherwise prior to returning. */ static int lookup_open (struct nameidata *nd,struct path *path,struct file *file,const struct open_flags *op, -bool got_write,int *opened) +bool got_write) {struct dentry *dir = nd->path.dentry;struct inode *dir_inode = dir->d_inode;
@@ -3153,7 +3149,7 @@ static int lookup_open(struct nameidata *nd, struct path *path,if (dir_inode->i_op->atomic_open) { error =atomic_open (nd, dentry, path, file, op, open_flag, - mode, opened); + mode);if (unlikely (error == -ENOENT ) && create_error) error = create_error;return error;
@@ -3206,8 +3202,7 @@ static int lookup_open(struct nameidata *nd, struct path *path,* Handle the last step of open() */ static int do_last (struct nameidata *nd, -struct file *file,const struct open_flags *op, -int *opened) +struct file *file,const struct open_flags *op) {struct dentry *dir = nd->path.dentry;int open_flag = op->open_flag;
@@ -3273,7 +3268,7 @@ static int do_last(struct nameidata *nd,inode_lock (dir->d_inode); else inode_lock_shared(dir->d_inode); - error =lookup_open (nd, &path, file, op, got_write, opened); + error =lookup_open (nd, &path, file, op, got_write);if (open_flag &O_CREAT )inode_unlock (dir->d_inode);else
@@ -3418,7 +3413,7 @@ EXPORT_SYMBOL(vfs_tmpfile);static int do_tmpfile (struct nameidata *nd,unsigned flags,const struct open_flags *op, -struct file *file,int *opened) +struct file *file) {struct dentry *child;struct path path;
@@ -3465,7 +3460,6 @@ static struct file *path_openat(struct nameidata *nd, {const char *s;struct file *file; -int opened =0 ;int error; file =get_empty_filp ();
@@ -3475,7 +3469,7 @@ static struct file *path_openat(struct nameidata *nd, file->f_flags = op->open_flag;if (unlikely (file->f_flags & __O_TMPFILE)) { - error =do_tmpfile (nd, flags, op, file, &opened); + error =do_tmpfile (nd, flags, op, file); goto out2; }
@@ -3490,7 +3484,7 @@ static struct file *path_openat(struct nameidata *nd,return ERR_CAST (s); }while (!(error =link_path_walk (s, nd)) && - (error =do_last (nd, file, op, &opened)) >0 ) { + (error =do_last (nd, file, op)) >0 ) { nd->flags &= ~(LOOKUP_OPEN |LOOKUP_CREATE |LOOKUP_EXCL ); s =trailing_symlink (nd);if (IS_ERR (s)) {
Signature · 1 line
2.11.0
I'm obviously biased since I asked for this, but:
Quoted 2 lines · Al Viro
33 files changed, 135 insertions(+), 180 deletions(-)
this already looks nice.
I'll go through the individual patches and see if there's anything there that raises my hackles. Silence will mean assent in this case
Linus
Al Viro
maintainerQuoted 10 lines · Linus Torvalds
I'm obviously biased since I asked for this, but:
Quoted 2 lines · Al Viro
33 files changed, 135 insertions(+), 180 deletions(-)this already looks nice.
I'll go through the individual patches and see if there's anything there that raises my hackles. Silence will mean assent in this case
BTW, looking through alloc_file() callers - in cxl_getfile() we have
try to grab f_op owner
try to grab fs_type
if failed => err_module
allocate an inode, set it up if successful
if failed => err_fs
allocate a dentry
if failed => err_inode
pin vfsmount
d_instantiate
alloc_file
if failed => err_dput
finish setting up
return fileerr_dput:
drop vfsmount/dentryerr_inode:
drop inodeerr_fs:
drop fs_typeerr_module:
drop f_op owner
return an error
That's a double iput when we hit alloc_file failure... There's a bunch of callers that can be massaged into something along such lines (not sharing that bug, though) and I wonder if we would be better off with wrapper like
something(inode, mnt, name, fops, mode)
{
struct qstr this = QSTR_INIT(name, strlen(name));
struct path path;
struct file *file;
path.dentry = d_alloc_anon(mnt->mnt_sb, &this);
if (!path.dentry)
return ERR_PTR(-ENOMEM);
path.mnt = mntget(mnt);
d_instantiate(path.dentry, inode);
file = alloc_file(&path, mode | FMODE_OPENED, fops);
if (IS_ERR(file)) {
ihold(inode);
path_put(&path);
}
return file;
}with users being
allocate inode
if failed => bugger off
set inode up
file = something(inode, mnt, name, fops, mode); if (IS_ERR(file))
drop inode, bugger off
finish setting file up
sock_alloc_file(): inode is coallocated with socket, otherwise it's as above -
struct file *file;
if (!dname) {
if (sock->sk)
dname = sock->sk->sk_prot_creator->name;
else
dname = "";
}
file = something(SOCK_INODE(sock), sock_mnt, dname,
�&socket_file_ops, FMODE_READ | FMODE_WRITE);
if (IS_ERR(file)) {
sock_release(sock);
return file;
}
sock->file = file;
file->f_flags = O_RDWR | (flags & O_NONBLOCK);
file->private_data = sock;
return file;aio_private_file(): exactly that form, turns into
struct file *file;
struct inode *inode = alloc_anon_inode(aio_mnt->mnt_sb);
if (IS_ERR(inode))
return ERR_CAST(inode);
inode->i_mapping->a_ops = &aio_ctx_aops;
inode->i_mapping->private_data = ctx;
inode->i_size = PAGE_SIZE * nr_pages;
file = something(inode, aio_mnt, "[aio]", &aio_ring_fops,
FMODE_READ | FMODE_WRITE);
if (IS_ERR(file))
iput(inode);
else
file->f_flags = O_RDWR;
return file;cxl_getfile(): after fixing the double-iput() in there, turns into
struct file *file;
struct inode *inode;
int rc;
if (fops->owner && !try_module_get(fops->owner))
return ERR_PTR(-ENOENT);
rc = simple_pin_fs(&cxl_fs_type, &cxl_vfs_mount, &cxl_fs_cnt);
if (rc < 0) {
pr_err("Cannot mount cxl pseudo filesystem: %d\n", rc);
file = ERR_PTR(rc);
goto err_module;
}
inode = alloc_anon_inode(cxl_vfs_mount->mnt_sb);
if (IS_ERR(inode)) {
file = ERR_CAST(inode);
goto err_fs;
}
file = something(inode, cxl_vfs_mount, name, fops, OPEN_FMODE(flags));
if (IS_ERR(file)) {
iput(inode);
goto err_fs;
}
file->f_flags = flags & (O_ACCMODE | O_NONBLOCK);
file->private_data = priv;
return file;err_fs:
simple_release_fs(&cxl_vfs_mount, &cxl_fs_cnt);
err_module:
module_put(fops->owner);
return file;__shmem_file_setup() - massaged into
struct inode *inode;
struct file *res;
if (IS_ERR(mnt))
return ERR_CAST(mnt);
if (size < 0 || size > MAX_LFS_FILESIZE)
return ERR_PTR(-EINVAL);
if (shmem_acct_size(flags, size))
return ERR_PTR(-ENOMEM);
inode = shmem_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0, flags);
if (unlikely(!inode)) {
shmem_unacct_size(flags, size);
return ERR_PTR(-ENOSPC);
}
inode->i_flags |= i_flags;
inode->i_size = size;
clear_nlink(inode); /* It is unlinked */
res = ERR_PTR(ramfs_nommu_expand_for_mapping(inode, size));
if (!IS_ERR(res))
res = something(inode, mnt, name, &shmem_file_operations,
FMODE_WRITE | FMODE_READ);
if (IS_ERR(res))
iput(inode);
return res;(massage includes setting ->s_d_op to hybrid of simple_dentry_operations and anon_ops).
hugetlb_file_setup() - massaged into
struct file *file;
struct inode *inode;
struct vfsmount *mnt;
int hstate_idx = get_hstate_idx(page_size_log);
if (hstate_idx < 0)
return ERR_PTR(-ENODEV);
*user = NULL;
mnt = hugetlbfs_vfsmount[hstate_idx];
if (!mnt)
return ERR_PTR(-ENOENT);
if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
*user = current_user();
if (user_shm_lock(size, *user)) {
task_lock(current);
pr_warn_once("%s (%d): Using mlock ulimits for SHM_HUGETLB is deprecated\n",
current->comm, current->pid);
task_unlock(current);
} else {
*user = NULL;
return ERR_PTR(-EPERM);
}
}
inode = hugetlbfs_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0);
if (unlikely(!inode)) {
file = ERR_PTR(-ENOSPC);
goto out;
}
if (creat_flags == HUGETLB_SHMFS_INODE)
inode->i_flags |= S_PRIVATE;
clear_nlink(inode);
inode->i_size = size;
if (hugetlb_reserve_pages(inode, 0,
size >> huge_page_shift(hstate_inode(inode)), NULL,
acctflag))
file = ERR_PTR(-ENOMEM);
else
file = something(inode, mnt, name, &hugetlbfs_file_operations,
FMODE_WRITE | FMODE_READ);
if (!IS_ERR(file))
return file;
out:
iput(inode);
if (*user) {
user_shm_unlock(size, *user);
*user = NULL;
}
return file;and the first caller of alloc_file() in create_pipe_files() also massages into similar form.
That leaves
* anon_inode_getfile() - converts to similar form, at the price of
ihold done slightly earlier, so that failure exit needs a (non-final, i.e. very cheap) iput() we currently avoid. Not a problem.
* do_shmat() and the second alloc_file() in create_pipe_files().
Those are rather different - we *do* have an existing dentry/inode/mount there and all we want on cleanup is path_put() to undo the path_get() we'd done.
* perfmon mess - _very_ different, and I wouldn't bet a dime on
correctness of failure exits there. One of the issues is that it simulates mmap as part of setup, so cleanup really is different.
AFAICS, there's a clear case for alloc_file() wrapper - 6 callers out of 10 get simpler with it, and the seventh is also a good candidate for the same treatment. Any naming ideas for that thing ("something" in the above) would be welcome...
BTW, that's almost all callers of d_alloc_pseudo() - there is exactly one caller not of that form (in __ns_get_path()) right now. perfmon should be another caller, but that might end up converted to the new wrapper...
As for put_filp()... the callers left in my local tree right now are
* path_openat(), dentry_open(), file_clone_open() (all of the
same form - "put_filp() if it doesn't have FMODE_OPENED, fput() otherwise)
* perfmon mess.create_pipe_files() got converted to fput() with a bit of massage...
Al Viro
maintainerQuoted 21 lines · Al Viro
That leaves
*anon_inode_getfile()- converts to similar form, at the price of
ihold done slightly earlier, so that failure exit needs a (non-final, i.e. very cheap)iput()we currently avoid. Not a problem.
*do_shmat()and the secondalloc_file()increate_pipe_files().
Those are rather different - we *do* have an existing dentry/inode/mount there and all we want on cleanup ispath_put()to undo thepath_get()we'd done.
* perfmon mess - _very_ different, and I wouldn't bet a dime on
correctness of failure exits there. One of the issues is that it simulates mmap as part of setup, so cleanup really is different.AFAICS, there's a clear case for
alloc_file()wrapper - 6 callers out of 10 get simpler with it, and the seventh is also a good candidate for the same treatment. Any naming ideas for that thing ("something" in the above) would be welcome...BTW, that's almost all callers of
d_alloc_pseudo()- there is exactly one caller not of that form (in__ns_get_path()) right now. perfmon should be another caller, but that might end up converted to the new wrapper...
Quoted 5 lines
As for
put_filp()... the callers left in my local tree right now are
*path_openat(),dentry_open(),file_clone_open()(all of the
same form - "put_filp()if it doesn't haveFMODE_OPENED,fput()otherwise)
* perfmon mess.create_pipe_files()got converted tofput()with a bit of massage...
Untested followup along those lines pushed; helper called alloc_file_pseudo() and all but 3 callers of alloc_file() got converted to it. perfmon is not touched and it's becoming more and more annoying ;-/ It's also sticking its tender bits into mm/* a lot - what it tries to do, AFAICS, is a heavily open-coded vmalloc-backed mmap() of sorts. It feels like there ought to be a more idiomatic way of doing that kind of stuff...
Stephane, could you comment on the situation in there? I realize that you hadn't touched that thing in more than a decade, but I've no idea who else might be familiar with that thing and it's very inconveniently special...
Al Viro
maintainerperfmon trouble
Quoted 3 lines · Al Viro
Stephane, could you comment on the situation in there? I realize that you hadn't touched that thing in more than a decade, but I've no idea who else might be familiar with that thing and it's very inconveniently special...
Having looked through that code... ouch. It tries to have munmap-on-close, of all things. Which has interesting consequences; consider, for example,
fd = perfctl(-1, PFM_CREATE_CONTEXT, &blah, 1); // create a context
....
pid = fork();
if (!pid) {
execve("/usr/bin/something_suid", ...);
...
}with something_suid(8) doing an explicit "close each descriptor past stdout" loop.
PFM_CREATE_CONTEXT has created a context, mmapped its buffer (and stored the address of that mapping in ctx->ctx_smpl_vaddr) and, having opened an associated file, sticks it into descriptor table and returns the descriptor.
On fork/exec we have
* descriptor table copied to child
* all mappings copied to child and then destroyed by execve
* execve ends up with the new binary (and libraries, etc.) mmapped
(in child)
Now, our careful suid-root binary does close(2) on its copy of descriptor. pfm_flush() is called. ctx->task != current, so we proceed to
/*
* remove virtual mapping, if any, for the calling task.
* cannot reset ctx field until last user is calling close().
*
* ctx_smpl_vaddr must never be cleared because it is needed
* by every task with access to the context
*
* When called from do_exit(), the mm context is gone already, therefore
* mm is NULL, i.e., the VMA is already gone and we do not have to
* do anything here
*/
if (ctx->ctx_smpl_vaddr && current->mm) {
smpl_buf_vaddr = ctx->ctx_smpl_vaddr;
smpl_buf_size = ctx->ctx_smpl_size;
}
UNPROTECT_CTX(ctx, flags);
/*
* if there was a mapping, then we systematically remove it
* at this point. Cannot be done inside critical section
* because some VM function reenables interrupts.
*
*/
if (smpl_buf_vaddr) pfm_remove_smpl_mapping(smpl_buf_vaddr, smpl_buf_size);... with the last call doing vm_munmap() on the area in question. In the address space of that suid-root binary, taking out whatever *it* had mapped at that address range...
I wouldn't be surprised if that turned out to be realistically exploitable ;-/ Is there any documentation of that thing's semantics? perfmonctl(2) doesn't mention the mapping at all and link to HP site in the arch/ia64/kernel/perfmon.c is 404-compliant. Playing with archive.org brings a sourceforget reference, but I hadn't been able to find anything ia64-related docs in there...
Re: perfmon trouble
Is perfmon even in still in use? If not it might be a good idea to drop it for good.
Re: perfmon trouble
Quoted 2 lines · Christoph Hellwig
Is perfmon even in still in use? If not it might be a good idea to drop it for good.
The problem is that even oprofile on ia64 depends on perfmon.
We have about four options, none of which are terribly appealing:
1. rm -rf arch/ia64. Bit harsh to do this when hardware is still being produced.
2. rm -rf arch/ia64's support for profiling completely. Would anybody complain?
3. Convert ia64 over to perf interface. Who would do this work?
4. Fix the perfmon kernel and userspace to not assume this insane semantic. Flag day, blah, blah.
Gentoo and Debian seem to be the only remaining living ports to Itanium and at least the Debian one is very unofficial.
Re: perfmon trouble
Quoted 2 lines · Matthew Wilcox
The problem is that even oprofile on ia64 depends on perfmon.
Hmm? You can definitely enable ia64 support for oprofile even without perfmon.
Are you saying the end result isn't usable?
Because I'd be inclined to just remove CONFIG_PERFMON support, and see if anybody even notices..
I'm not expecting a lot of people to do a lot of oprofile on ia64 anyway. It's a bit late to start optimizing things now.
Do people use perfmon still? Maybe. Maybe not. Perhaps we could just mark it as broken in the Kconfig file for now, and see if somebody says something?
Linus
Re: perfmon trouble
Quoted 5 lines · Linus Torvalds
Quoted 2 lines · Matthew Wilcox
The problem is that even oprofile on ia64 depends on perfmon.
Hmm? You can definitely enable ia64 support for oprofile even without perfmon.
Oh, I think my memory is playing tricks on me. This is my confusion, I think:
oprofile-$(CONFIG_PERFMON) += perfmon.o
so perfmon events are exposed through oprofile, but you can disable perfmon without disabling oprofile.
Quoted 9 lines
Because I'd be inclined to just remove
CONFIG_PERFMONsupport, and see if anybody even notices..I'm not expecting a lot of people to do a lot of oprofile on ia64 anyway. It's a bit late to start optimizing things now.
Do people use perfmon still? Maybe. Maybe not. Perhaps we could just mark it as broken in the
Kconfigfile for now, and see if somebody says something?
That gets my vote.
Tony? Fenghua?
arch/ia64/Kconfig +1 -0
@@ -455,6 +455,7 @@ config IA64_MCA_RECOVERY config PERFMON bool "Performance monitor support" + depends on BROKEN help Selects whether support for the IA-64 performance monitor hardware is included in the kernel. This makes some kernel data-structures a
Re: perfmon trouble
Hi,
Quoted 6 lines · Matthew Wilcox
Quoted 4 lines · Linus Torvalds
Quoted 2 lines · Matthew Wilcox
The problem is that even oprofile on ia64 depends on perfmon.
Quoted 19 lines
Quoted 1 line
Hmm? You can definitely enable ia64 support for oprofile even without perfmon.
Oh, I think my memory is playing tricks on me. This is my confusion, I think:
oprofile-$(CONFIG_PERFMON) += perfmon.o
so perfmon events are exposed through oprofile, but you can disable perfmon without disabling oprofile.
Quoted 9 lines
Because I'd be inclined to just remove
CONFIG_PERFMONsupport, and see if anybody even notices..I'm not expecting a lot of people to do a lot of oprofile on ia64 anyway. It's a bit late to start optimizing things now.
Do people use perfmon still? Maybe. Maybe not. Perhaps we could just mark it as broken in the
Kconfigfile for now, and see if somebody says something?
Thanks Al for the careful analysis. If I understand correctly, the problem seems to be that on
execve the mapping of the sampling buffer is not destroyed and then on close, the kernel
unmaps whatever the new binary had in that address range. The fix would be to ensure
the mmap is destroyed on execve.
My problem is that I do not have IA64 hw anymore, so whatever the fix, I will not be able
to test this. In the meantime, I agree with Matthew, simply disable PERFMON support.
thanks.
Quoted 17 lines
That gets my vote.
Tony? Fenghua?
arch/ia64/Kconfig +1
-0@@ -455,6 +455,7 @@ config IA64_MCA_RECOVERY config PERFMON bool "Performance monitor support" + depends on BROKEN help Selects whether support for the IA-64 performance monitor hardware is included in the kernel. This makes some kernel data-structures a
Re: perfmon trouble
Quoted 3 lines · Stephane Eranian
In the meantime, I agree with Matthew, simply disable PERFMON support.
Ok, since everybody seems to agree on that, I marked it BROKEN for now, in the hope that nobody even notices and we can just remove the code entirely.
And if somebody *does* notice, at least we would also have a sucker^Wtester for any possible patches to fix it.
Linus
Al Viro
maintainerRe: perfmon trouble
In reply to Stephane EranianQuoted 7 lines · Stephane Eranian
Thanks Al for the careful analysis. If I understand correctly, the problem seems to be that on
execve the mapping of the sampling buffer is not destroyed and then on close, the kernel
unmaps whatever the new binary had in that address range. The fix would be to ensure
the mmap is destroyed on execve.
mmap *is* destroyed on execve; it's just that perfmon doesn't notice and then tries to destroy it again on close, nevermind that it's a different process, different mapping, etc.
Quoted 4 lines
My problem is that I do not have IA64 hw anymore, so whatever the fix, I will not be able
to test this. In the meantime, I agree with Matthew, simply disable PERFMON support.
Out of curiosity - what happened to never-went-into-mainline rewrite of perfmon circa 2008? perfmon2 sourceforget site has a collection of those, up to 2.6.29; AFAICS, they did have a somewhat saner syscall interface (no auto-mmap on creation, no auto-munmap attempts on close). By this point it obviously would've been too late to resurrect anyway, but I wonder what happened with it...
Al Viro
maintainerQuoted 13 lines · Al Viro
Quoted 10 lines · Linus Torvalds
I'm obviously biased since I asked for this, but:
Quoted 2 lines · Al Viro
33 files changed, 135 insertions(+), 180 deletions(-)this already looks nice.
I'll go through the individual patches and see if there's anything there that raises my hackles. Silence will mean assent in this case
BTW, looking through
alloc_file()callers - incxl_getfile()we have
[snip]
Quoted 4 lines
That's a double iput when we hit
alloc_filefailure... There's a bunch of callers that can be massaged into something along such lines (not sharing that bug, though) and I wonder if we would be better off with wrapper like
... and call-by-editor strikes again - this window we've got it cut'n'pasted into ocxlflash_getfile(), with the same double iput in place...