| Message ID | 1366977861-27678-3-git-send-email-piastry@etersoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 26 Apr 2013 16:04:16 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > Introduce new LOCK_DELETE flock flag that is suggested to be used > internally only to map O_DENYDELETE open flag: > > !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- > fs/namei.c | 3 +++ > include/linux/fs.h | 6 +++++ > include/uapi/asm-generic/fcntl.h | 1 + > 4 files changed, 54 insertions(+), 9 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index dbc5557..1cc68a9 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); > > static inline int flock_translate_cmd(int cmd) { > if (cmd & LOCK_MAND) > - return cmd & (LOCK_MAND | LOCK_RW); > + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); > switch (cmd) { > case LOCK_SH: > return F_RDLCK; > @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) > cmd |= LOCK_READ; > if (!(flags & O_DENYWRITE)) > cmd |= LOCK_WRITE; > + if (!(flags & O_DENYDELETE)) > + cmd |= LOCK_DELETE; > > return cmd; > } > @@ -836,6 +838,31 @@ out: > return error; > } > > +int > +sharelock_may_delete(struct dentry *dentry) > +{ > + struct file_lock **before; > + int rc = 0; > + > + if (!IS_SHARELOCK(dentry->d_inode)) > + return rc; > + > + lock_flocks(); > + for_each_lock(dentry->d_inode, before) { > + struct file_lock *fl = *before; > + if (IS_POSIX(fl)) > + break; > + if (IS_LEASE(fl)) > + continue; > + if (fl->fl_type & LOCK_DELETE) > + continue; > + rc = 1; > + break; > + } > + unlock_flocks(); > + return rc; > +} > + > /* > * Determine if a file is allowed to be opened with specified access and share > * modes. Lock the file and return 0 if checks passed, otherwise return > @@ -850,10 +877,6 @@ sharelock_lock_file(struct file *filp) > if (!IS_SHARELOCK(filp->f_path.dentry->d_inode)) > return error; > > - /* Disable O_DENYDELETE support for now */ > - if (filp->f_flags & O_DENYDELETE) > - return -EINVAL; > - > error = flock_make_lock(filp, &lock, deny_flags_to_cmd(filp->f_flags)); > if (error) > return error; > @@ -1717,6 +1740,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > if (!f.file) > goto out; > > + /* LOCK_DELETE is defined to be translated from O_DENYDELETE only */ > + if (cmd & LOCK_DELETE) { > + error = -EINVAL; > + goto out; > + } > + > can_sleep = !(cmd & LOCK_NB); > cmd &= ~LOCK_NB; > unlock = (cmd == LOCK_UN); > @@ -2261,10 +2290,16 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > seq_printf(f, "UNKNOWN UNKNOWN "); > } > if (fl->fl_type & LOCK_MAND) { > - seq_printf(f, "%s ", > - (fl->fl_type & LOCK_READ) > - ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " > - : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > + if (fl->fl_type & LOCK_DELETE) > + seq_printf(f, "%s ", > + (fl->fl_type & LOCK_READ) ? > + (fl->fl_type & LOCK_WRITE) ? "RWDEL" : "RDDEL" : > + (fl->fl_type & LOCK_WRITE) ? "WRDEL" : "DEL "); > + else > + seq_printf(f, "%s ", > + (fl->fl_type & LOCK_READ) ? > + (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " : > + (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > } else { > seq_printf(f, "%s ", > (lease_breaking(fl)) > diff --git a/fs/namei.c b/fs/namei.c > index dd236fe..a404f7d 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2220,6 +2220,7 @@ static inline int check_sticky(struct inode *dir, struct inode *inode) > * 9. We can't remove a root or mountpoint. > * 10. We don't allow removal of NFS sillyrenamed files; it's handled by > * nfs_async_unlink(). > + * 11. We can't do it if victim is locked by O_DENYDELETE sharelock. > */ > static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > { > @@ -2250,6 +2251,8 @@ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > return -ENOENT; > if (victim->d_flags & DCACHE_NFSFS_RENAMED) > return -EBUSY; > + if (sharelock_may_delete(victim)) > + return -ESHAREDENIED; Is there a potential race here? You're holding the parent's i_mutex when setting a lock on this file, but you're not holding it when you test for it here. So it seems possible you could end up granting a O_DENYDELETE open on a file that is in the process of being deleted from the namespace. > return 0; > } > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 24066d2..afd56b1 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1006,6 +1006,7 @@ extern int lock_may_read(struct inode *, loff_t start, unsigned long count); > extern int lock_may_write(struct inode *, loff_t start, unsigned long count); > extern void locks_delete_block(struct file_lock *waiter); > extern int sharelock_lock_file(struct file *); > +extern int sharelock_may_delete(struct dentry *); > extern void lock_flocks(void); > extern void unlock_flocks(void); > #else /* !CONFIG_FILE_LOCKING */ > @@ -1159,6 +1160,11 @@ static inline int sharelock_lock_file(struct file *filp) > return 0; > } > > +static inline int sharelock_may_delete(struct dentry *dentry) > +{ > + return 0; > +} > + > static inline void lock_flocks(void) > { > } > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index 5ac0d49..a3e6349 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -167,6 +167,7 @@ struct f_owner_ex { > blocking */ > #define LOCK_UN 8 /* remove lock */ > > +#define LOCK_DELETE 16 /* which allows to delete a file */ > #define LOCK_MAND 32 /* This is a mandatory flock ... */ > #define LOCK_READ 64 /* which allows concurrent read operations */ > #define LOCK_WRITE 128 /* which allows concurrent write operations */
2013/5/10 Jeff Layton <jlayton@poochiereds.net>: > On Fri, 26 Apr 2013 16:04:16 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> Introduce new LOCK_DELETE flock flag that is suggested to be used >> internally only to map O_DENYDELETE open flag: >> >> !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. >> >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> --- >> fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- >> fs/namei.c | 3 +++ >> include/linux/fs.h | 6 +++++ >> include/uapi/asm-generic/fcntl.h | 1 + >> 4 files changed, 54 insertions(+), 9 deletions(-) >> >> diff --git a/fs/locks.c b/fs/locks.c >> index dbc5557..1cc68a9 100644 >> --- a/fs/locks.c >> +++ b/fs/locks.c >> @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); >> >> static inline int flock_translate_cmd(int cmd) { >> if (cmd & LOCK_MAND) >> - return cmd & (LOCK_MAND | LOCK_RW); >> + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); >> switch (cmd) { >> case LOCK_SH: >> return F_RDLCK; >> @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) >> cmd |= LOCK_READ; >> if (!(flags & O_DENYWRITE)) >> cmd |= LOCK_WRITE; >> + if (!(flags & O_DENYDELETE)) >> + cmd |= LOCK_DELETE; >> >> return cmd; >> } >> @@ -836,6 +838,31 @@ out: >> return error; >> } >> >> +int >> +sharelock_may_delete(struct dentry *dentry) >> +{ >> + struct file_lock **before; >> + int rc = 0; >> + >> + if (!IS_SHARELOCK(dentry->d_inode)) >> + return rc; >> + >> + lock_flocks(); >> + for_each_lock(dentry->d_inode, before) { >> + struct file_lock *fl = *before; >> + if (IS_POSIX(fl)) >> + break; >> + if (IS_LEASE(fl)) >> + continue; >> + if (fl->fl_type & LOCK_DELETE) >> + continue; >> + rc = 1; >> + break; >> + } >> + unlock_flocks(); >> + return rc; >> +} >> + >> /* >> * Determine if a file is allowed to be opened with specified access and share >> * modes. Lock the file and return 0 if checks passed, otherwise return >> @@ -850,10 +877,6 @@ sharelock_lock_file(struct file *filp) >> if (!IS_SHARELOCK(filp->f_path.dentry->d_inode)) >> return error; >> >> - /* Disable O_DENYDELETE support for now */ >> - if (filp->f_flags & O_DENYDELETE) >> - return -EINVAL; >> - >> error = flock_make_lock(filp, &lock, deny_flags_to_cmd(filp->f_flags)); >> if (error) >> return error; >> @@ -1717,6 +1740,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) >> if (!f.file) >> goto out; >> >> + /* LOCK_DELETE is defined to be translated from O_DENYDELETE only */ >> + if (cmd & LOCK_DELETE) { >> + error = -EINVAL; >> + goto out; >> + } >> + >> can_sleep = !(cmd & LOCK_NB); >> cmd &= ~LOCK_NB; >> unlock = (cmd == LOCK_UN); >> @@ -2261,10 +2290,16 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, >> seq_printf(f, "UNKNOWN UNKNOWN "); >> } >> if (fl->fl_type & LOCK_MAND) { >> - seq_printf(f, "%s ", >> - (fl->fl_type & LOCK_READ) >> - ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " >> - : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); >> + if (fl->fl_type & LOCK_DELETE) >> + seq_printf(f, "%s ", >> + (fl->fl_type & LOCK_READ) ? >> + (fl->fl_type & LOCK_WRITE) ? "RWDEL" : "RDDEL" : >> + (fl->fl_type & LOCK_WRITE) ? "WRDEL" : "DEL "); >> + else >> + seq_printf(f, "%s ", >> + (fl->fl_type & LOCK_READ) ? >> + (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " : >> + (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); >> } else { >> seq_printf(f, "%s ", >> (lease_breaking(fl)) >> diff --git a/fs/namei.c b/fs/namei.c >> index dd236fe..a404f7d 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -2220,6 +2220,7 @@ static inline int check_sticky(struct inode *dir, struct inode *inode) >> * 9. We can't remove a root or mountpoint. >> * 10. We don't allow removal of NFS sillyrenamed files; it's handled by >> * nfs_async_unlink(). >> + * 11. We can't do it if victim is locked by O_DENYDELETE sharelock. >> */ >> static int may_delete(struct inode *dir,struct dentry *victim,int isdir) >> { >> @@ -2250,6 +2251,8 @@ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) >> return -ENOENT; >> if (victim->d_flags & DCACHE_NFSFS_RENAMED) >> return -EBUSY; >> + if (sharelock_may_delete(victim)) >> + return -ESHAREDENIED; > > > Is there a potential race here? > > You're holding the parent's i_mutex when setting a lock on this file, > but you're not holding it when you test for it here. So it seems > possible you could end up granting a O_DENYDELETE open on a file that > is in the process of being deleted from the namespace. may_delete function is called from vfs_unlnk, vfs_rename and vfs_rmdir and in all those places the caller is holding parent's i_mutex. It seems that the locking order is correct: we hold parent's i_mutex when we set and test sharelocks. >> return 0; >> } >> >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 24066d2..afd56b1 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -1006,6 +1006,7 @@ extern int lock_may_read(struct inode *, loff_t start, unsigned long count); >> extern int lock_may_write(struct inode *, loff_t start, unsigned long count); >> extern void locks_delete_block(struct file_lock *waiter); >> extern int sharelock_lock_file(struct file *); >> +extern int sharelock_may_delete(struct dentry *); >> extern void lock_flocks(void); >> extern void unlock_flocks(void); >> #else /* !CONFIG_FILE_LOCKING */ >> @@ -1159,6 +1160,11 @@ static inline int sharelock_lock_file(struct file *filp) >> return 0; >> } >> >> +static inline int sharelock_may_delete(struct dentry *dentry) >> +{ >> + return 0; >> +} >> + >> static inline void lock_flocks(void) >> { >> } >> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h >> index 5ac0d49..a3e6349 100644 >> --- a/include/uapi/asm-generic/fcntl.h >> +++ b/include/uapi/asm-generic/fcntl.h >> @@ -167,6 +167,7 @@ struct f_owner_ex { >> blocking */ >> #define LOCK_UN 8 /* remove lock */ >> >> +#define LOCK_DELETE 16 /* which allows to delete a file */ >> #define LOCK_MAND 32 /* This is a mandatory flock ... */ >> #define LOCK_READ 64 /* which allows concurrent read operations */ >> #define LOCK_WRITE 128 /* which allows concurrent write operations */ > > > -- > Jeff Layton <jlayton@poochiereds.net> > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best regards, Pavel Shilovsky. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 13 May 2013 21:50:23 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > 2013/5/10 Jeff Layton <jlayton@poochiereds.net>: > > On Fri, 26 Apr 2013 16:04:16 +0400 > > Pavel Shilovsky <piastry@etersoft.ru> wrote: > > > >> Introduce new LOCK_DELETE flock flag that is suggested to be used > >> internally only to map O_DENYDELETE open flag: > >> > >> !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. > >> > >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > >> --- > >> fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- > >> fs/namei.c | 3 +++ > >> include/linux/fs.h | 6 +++++ > >> include/uapi/asm-generic/fcntl.h | 1 + > >> 4 files changed, 54 insertions(+), 9 deletions(-) > >> > >> diff --git a/fs/locks.c b/fs/locks.c > >> index dbc5557..1cc68a9 100644 > >> --- a/fs/locks.c > >> +++ b/fs/locks.c > >> @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); > >> > >> static inline int flock_translate_cmd(int cmd) { > >> if (cmd & LOCK_MAND) > >> - return cmd & (LOCK_MAND | LOCK_RW); > >> + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); > >> switch (cmd) { > >> case LOCK_SH: > >> return F_RDLCK; > >> @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) > >> cmd |= LOCK_READ; > >> if (!(flags & O_DENYWRITE)) > >> cmd |= LOCK_WRITE; > >> + if (!(flags & O_DENYDELETE)) > >> + cmd |= LOCK_DELETE; > >> > >> return cmd; > >> } > >> @@ -836,6 +838,31 @@ out: > >> return error; > >> } > >> > >> +int > >> +sharelock_may_delete(struct dentry *dentry) > >> +{ > >> + struct file_lock **before; > >> + int rc = 0; > >> + > >> + if (!IS_SHARELOCK(dentry->d_inode)) > >> + return rc; > >> + > >> + lock_flocks(); > >> + for_each_lock(dentry->d_inode, before) { > >> + struct file_lock *fl = *before; > >> + if (IS_POSIX(fl)) > >> + break; > >> + if (IS_LEASE(fl)) > >> + continue; > >> + if (fl->fl_type & LOCK_DELETE) > >> + continue; > >> + rc = 1; > >> + break; > >> + } > >> + unlock_flocks(); > >> + return rc; > >> +} > >> + > >> /* > >> * Determine if a file is allowed to be opened with specified access and share > >> * modes. Lock the file and return 0 if checks passed, otherwise return > >> @@ -850,10 +877,6 @@ sharelock_lock_file(struct file *filp) > >> if (!IS_SHARELOCK(filp->f_path.dentry->d_inode)) > >> return error; > >> > >> - /* Disable O_DENYDELETE support for now */ > >> - if (filp->f_flags & O_DENYDELETE) > >> - return -EINVAL; > >> - > >> error = flock_make_lock(filp, &lock, deny_flags_to_cmd(filp->f_flags)); > >> if (error) > >> return error; > >> @@ -1717,6 +1740,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > >> if (!f.file) > >> goto out; > >> > >> + /* LOCK_DELETE is defined to be translated from O_DENYDELETE only */ > >> + if (cmd & LOCK_DELETE) { > >> + error = -EINVAL; > >> + goto out; > >> + } > >> + > >> can_sleep = !(cmd & LOCK_NB); > >> cmd &= ~LOCK_NB; > >> unlock = (cmd == LOCK_UN); > >> @@ -2261,10 +2290,16 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > >> seq_printf(f, "UNKNOWN UNKNOWN "); > >> } > >> if (fl->fl_type & LOCK_MAND) { > >> - seq_printf(f, "%s ", > >> - (fl->fl_type & LOCK_READ) > >> - ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " > >> - : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > >> + if (fl->fl_type & LOCK_DELETE) > >> + seq_printf(f, "%s ", > >> + (fl->fl_type & LOCK_READ) ? > >> + (fl->fl_type & LOCK_WRITE) ? "RWDEL" : "RDDEL" : > >> + (fl->fl_type & LOCK_WRITE) ? "WRDEL" : "DEL "); > >> + else > >> + seq_printf(f, "%s ", > >> + (fl->fl_type & LOCK_READ) ? > >> + (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " : > >> + (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > >> } else { > >> seq_printf(f, "%s ", > >> (lease_breaking(fl)) > >> diff --git a/fs/namei.c b/fs/namei.c > >> index dd236fe..a404f7d 100644 > >> --- a/fs/namei.c > >> +++ b/fs/namei.c > >> @@ -2220,6 +2220,7 @@ static inline int check_sticky(struct inode *dir, struct inode *inode) > >> * 9. We can't remove a root or mountpoint. > >> * 10. We don't allow removal of NFS sillyrenamed files; it's handled by > >> * nfs_async_unlink(). > >> + * 11. We can't do it if victim is locked by O_DENYDELETE sharelock. > >> */ > >> static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > >> { > >> @@ -2250,6 +2251,8 @@ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > >> return -ENOENT; > >> if (victim->d_flags & DCACHE_NFSFS_RENAMED) > >> return -EBUSY; > >> + if (sharelock_may_delete(victim)) > >> + return -ESHAREDENIED; > > > > > > Is there a potential race here? > > > > You're holding the parent's i_mutex when setting a lock on this file, > > but you're not holding it when you test for it here. So it seems > > possible you could end up granting a O_DENYDELETE open on a file that > > is in the process of being deleted from the namespace. > > may_delete function is called from vfs_unlnk, vfs_rename and vfs_rmdir > and in all those places the caller is holding parent's i_mutex. It > seems that the locking order is correct: we hold parent's i_mutex when > we set and test sharelocks. > You're correct -- I misread the code. They do hold the parent's i_mutex in all of the critical spots, so that should prevent the above race.
On Fri, 26 Apr 2013 16:04:16 +0400 Pavel Shilovsky <piastry@etersoft.ru> wrote: > Introduce new LOCK_DELETE flock flag that is suggested to be used > internally only to map O_DENYDELETE open flag: > > !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. > > Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> > --- > fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- > fs/namei.c | 3 +++ > include/linux/fs.h | 6 +++++ > include/uapi/asm-generic/fcntl.h | 1 + > 4 files changed, 54 insertions(+), 9 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index dbc5557..1cc68a9 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); > > static inline int flock_translate_cmd(int cmd) { > if (cmd & LOCK_MAND) > - return cmd & (LOCK_MAND | LOCK_RW); > + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); > switch (cmd) { > case LOCK_SH: > return F_RDLCK; > @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) > cmd |= LOCK_READ; > if (!(flags & O_DENYWRITE)) > cmd |= LOCK_WRITE; > + if (!(flags & O_DENYDELETE)) > + cmd |= LOCK_DELETE; > > return cmd; > } > @@ -836,6 +838,31 @@ out: > return error; > } > > +int > +sharelock_may_delete(struct dentry *dentry) > +{ > + struct file_lock **before; > + int rc = 0; > + > + if (!IS_SHARELOCK(dentry->d_inode)) > + return rc; > + > + lock_flocks(); > + for_each_lock(dentry->d_inode, before) { > + struct file_lock *fl = *before; > + if (IS_POSIX(fl)) > + break; > + if (IS_LEASE(fl)) > + continue; > + if (fl->fl_type & LOCK_DELETE) > + continue; Are you sure this logic is right? What if I have a normal non-LOCK_MAND lock on this file. Won't that prevent me from deleting it with this patch? > + rc = 1; > + break; > + } > + unlock_flocks(); > + return rc; > +} > + > /* > * Determine if a file is allowed to be opened with specified access and share > * modes. Lock the file and return 0 if checks passed, otherwise return > @@ -850,10 +877,6 @@ sharelock_lock_file(struct file *filp) > if (!IS_SHARELOCK(filp->f_path.dentry->d_inode)) > return error; > > - /* Disable O_DENYDELETE support for now */ > - if (filp->f_flags & O_DENYDELETE) > - return -EINVAL; > - > error = flock_make_lock(filp, &lock, deny_flags_to_cmd(filp->f_flags)); > if (error) > return error; > @@ -1717,6 +1740,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > if (!f.file) > goto out; > > + /* LOCK_DELETE is defined to be translated from O_DENYDELETE only */ > + if (cmd & LOCK_DELETE) { > + error = -EINVAL; > + goto out; > + } > + > can_sleep = !(cmd & LOCK_NB); > cmd &= ~LOCK_NB; > unlock = (cmd == LOCK_UN); > @@ -2261,10 +2290,16 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > seq_printf(f, "UNKNOWN UNKNOWN "); > } > if (fl->fl_type & LOCK_MAND) { > - seq_printf(f, "%s ", > - (fl->fl_type & LOCK_READ) > - ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " > - : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > + if (fl->fl_type & LOCK_DELETE) > + seq_printf(f, "%s ", > + (fl->fl_type & LOCK_READ) ? > + (fl->fl_type & LOCK_WRITE) ? "RWDEL" : "RDDEL" : > + (fl->fl_type & LOCK_WRITE) ? "WRDEL" : "DEL "); > + else > + seq_printf(f, "%s ", > + (fl->fl_type & LOCK_READ) ? > + (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " : > + (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); > } else { > seq_printf(f, "%s ", > (lease_breaking(fl)) > diff --git a/fs/namei.c b/fs/namei.c > index dd236fe..a404f7d 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2220,6 +2220,7 @@ static inline int check_sticky(struct inode *dir, struct inode *inode) > * 9. We can't remove a root or mountpoint. > * 10. We don't allow removal of NFS sillyrenamed files; it's handled by > * nfs_async_unlink(). > + * 11. We can't do it if victim is locked by O_DENYDELETE sharelock. > */ > static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > { > @@ -2250,6 +2251,8 @@ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) > return -ENOENT; > if (victim->d_flags & DCACHE_NFSFS_RENAMED) > return -EBUSY; > + if (sharelock_may_delete(victim)) > + return -ESHAREDENIED; > return 0; > } > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 24066d2..afd56b1 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1006,6 +1006,7 @@ extern int lock_may_read(struct inode *, loff_t start, unsigned long count); > extern int lock_may_write(struct inode *, loff_t start, unsigned long count); > extern void locks_delete_block(struct file_lock *waiter); > extern int sharelock_lock_file(struct file *); > +extern int sharelock_may_delete(struct dentry *); > extern void lock_flocks(void); > extern void unlock_flocks(void); > #else /* !CONFIG_FILE_LOCKING */ > @@ -1159,6 +1160,11 @@ static inline int sharelock_lock_file(struct file *filp) > return 0; > } > > +static inline int sharelock_may_delete(struct dentry *dentry) > +{ > + return 0; > +} > + > static inline void lock_flocks(void) > { > } > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index 5ac0d49..a3e6349 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -167,6 +167,7 @@ struct f_owner_ex { > blocking */ > #define LOCK_UN 8 /* remove lock */ > > +#define LOCK_DELETE 16 /* which allows to delete a file */ > #define LOCK_MAND 32 /* This is a mandatory flock ... */ > #define LOCK_READ 64 /* which allows concurrent read operations */ > #define LOCK_WRITE 128 /* which allows concurrent write operations */
2013/6/11 Jeff Layton <jlayton@poochiereds.net>: > On Fri, 26 Apr 2013 16:04:16 +0400 > Pavel Shilovsky <piastry@etersoft.ru> wrote: > >> Introduce new LOCK_DELETE flock flag that is suggested to be used >> internally only to map O_DENYDELETE open flag: >> >> !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. >> >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> >> --- >> fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- >> fs/namei.c | 3 +++ >> include/linux/fs.h | 6 +++++ >> include/uapi/asm-generic/fcntl.h | 1 + >> 4 files changed, 54 insertions(+), 9 deletions(-) >> >> diff --git a/fs/locks.c b/fs/locks.c >> index dbc5557..1cc68a9 100644 >> --- a/fs/locks.c >> +++ b/fs/locks.c >> @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); >> >> static inline int flock_translate_cmd(int cmd) { >> if (cmd & LOCK_MAND) >> - return cmd & (LOCK_MAND | LOCK_RW); >> + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); >> switch (cmd) { >> case LOCK_SH: >> return F_RDLCK; >> @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) >> cmd |= LOCK_READ; >> if (!(flags & O_DENYWRITE)) >> cmd |= LOCK_WRITE; >> + if (!(flags & O_DENYDELETE)) >> + cmd |= LOCK_DELETE; >> >> return cmd; >> } >> @@ -836,6 +838,31 @@ out: >> return error; >> } >> >> +int >> +sharelock_may_delete(struct dentry *dentry) >> +{ >> + struct file_lock **before; >> + int rc = 0; >> + >> + if (!IS_SHARELOCK(dentry->d_inode)) >> + return rc; >> + >> + lock_flocks(); >> + for_each_lock(dentry->d_inode, before) { >> + struct file_lock *fl = *before; >> + if (IS_POSIX(fl)) >> + break; >> + if (IS_LEASE(fl)) >> + continue; >> + if (fl->fl_type & LOCK_DELETE) >> + continue; > > Are you sure this logic is right? What if I have a normal non-LOCK_MAND > lock on this file. Won't that prevent me from deleting it with this > patch? > It is a bug, thank you for pointing it out. We need to skip all non-LOCK_MAND locks here. -- Best regards, Pavel Shilovsky. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/locks.c b/fs/locks.c index dbc5557..1cc68a9 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -269,7 +269,7 @@ EXPORT_SYMBOL(locks_copy_lock); static inline int flock_translate_cmd(int cmd) { if (cmd & LOCK_MAND) - return cmd & (LOCK_MAND | LOCK_RW); + return cmd & (LOCK_MAND | LOCK_RW | LOCK_DELETE); switch (cmd) { case LOCK_SH: return F_RDLCK; @@ -614,6 +614,8 @@ deny_flags_to_cmd(unsigned int flags) cmd |= LOCK_READ; if (!(flags & O_DENYWRITE)) cmd |= LOCK_WRITE; + if (!(flags & O_DENYDELETE)) + cmd |= LOCK_DELETE; return cmd; } @@ -836,6 +838,31 @@ out: return error; } +int +sharelock_may_delete(struct dentry *dentry) +{ + struct file_lock **before; + int rc = 0; + + if (!IS_SHARELOCK(dentry->d_inode)) + return rc; + + lock_flocks(); + for_each_lock(dentry->d_inode, before) { + struct file_lock *fl = *before; + if (IS_POSIX(fl)) + break; + if (IS_LEASE(fl)) + continue; + if (fl->fl_type & LOCK_DELETE) + continue; + rc = 1; + break; + } + unlock_flocks(); + return rc; +} + /* * Determine if a file is allowed to be opened with specified access and share * modes. Lock the file and return 0 if checks passed, otherwise return @@ -850,10 +877,6 @@ sharelock_lock_file(struct file *filp) if (!IS_SHARELOCK(filp->f_path.dentry->d_inode)) return error; - /* Disable O_DENYDELETE support for now */ - if (filp->f_flags & O_DENYDELETE) - return -EINVAL; - error = flock_make_lock(filp, &lock, deny_flags_to_cmd(filp->f_flags)); if (error) return error; @@ -1717,6 +1740,12 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) if (!f.file) goto out; + /* LOCK_DELETE is defined to be translated from O_DENYDELETE only */ + if (cmd & LOCK_DELETE) { + error = -EINVAL; + goto out; + } + can_sleep = !(cmd & LOCK_NB); cmd &= ~LOCK_NB; unlock = (cmd == LOCK_UN); @@ -2261,10 +2290,16 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, seq_printf(f, "UNKNOWN UNKNOWN "); } if (fl->fl_type & LOCK_MAND) { - seq_printf(f, "%s ", - (fl->fl_type & LOCK_READ) - ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " - : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); + if (fl->fl_type & LOCK_DELETE) + seq_printf(f, "%s ", + (fl->fl_type & LOCK_READ) ? + (fl->fl_type & LOCK_WRITE) ? "RWDEL" : "RDDEL" : + (fl->fl_type & LOCK_WRITE) ? "WRDEL" : "DEL "); + else + seq_printf(f, "%s ", + (fl->fl_type & LOCK_READ) ? + (fl->fl_type & LOCK_WRITE) ? "RW " : "READ " : + (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE "); } else { seq_printf(f, "%s ", (lease_breaking(fl)) diff --git a/fs/namei.c b/fs/namei.c index dd236fe..a404f7d 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2220,6 +2220,7 @@ static inline int check_sticky(struct inode *dir, struct inode *inode) * 9. We can't remove a root or mountpoint. * 10. We don't allow removal of NFS sillyrenamed files; it's handled by * nfs_async_unlink(). + * 11. We can't do it if victim is locked by O_DENYDELETE sharelock. */ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) { @@ -2250,6 +2251,8 @@ static int may_delete(struct inode *dir,struct dentry *victim,int isdir) return -ENOENT; if (victim->d_flags & DCACHE_NFSFS_RENAMED) return -EBUSY; + if (sharelock_may_delete(victim)) + return -ESHAREDENIED; return 0; } diff --git a/include/linux/fs.h b/include/linux/fs.h index 24066d2..afd56b1 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1006,6 +1006,7 @@ extern int lock_may_read(struct inode *, loff_t start, unsigned long count); extern int lock_may_write(struct inode *, loff_t start, unsigned long count); extern void locks_delete_block(struct file_lock *waiter); extern int sharelock_lock_file(struct file *); +extern int sharelock_may_delete(struct dentry *); extern void lock_flocks(void); extern void unlock_flocks(void); #else /* !CONFIG_FILE_LOCKING */ @@ -1159,6 +1160,11 @@ static inline int sharelock_lock_file(struct file *filp) return 0; } +static inline int sharelock_may_delete(struct dentry *dentry) +{ + return 0; +} + static inline void lock_flocks(void) { } diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h index 5ac0d49..a3e6349 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -167,6 +167,7 @@ struct f_owner_ex { blocking */ #define LOCK_UN 8 /* remove lock */ +#define LOCK_DELETE 16 /* which allows to delete a file */ #define LOCK_MAND 32 /* This is a mandatory flock ... */ #define LOCK_READ 64 /* which allows concurrent read operations */ #define LOCK_WRITE 128 /* which allows concurrent write operations */
Introduce new LOCK_DELETE flock flag that is suggested to be used internally only to map O_DENYDELETE open flag: !O_DENYDELETE -> LOCK_DELETE | LOCK_MAND. Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru> --- fs/locks.c | 53 +++++++++++++++++++++++++++++++++------- fs/namei.c | 3 +++ include/linux/fs.h | 6 +++++ include/uapi/asm-generic/fcntl.h | 1 + 4 files changed, 54 insertions(+), 9 deletions(-)