From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f181.google.com ([209.85.220.181]:35254 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757989AbdCUSov (ORCPT ); Tue, 21 Mar 2017 14:44:51 -0400 Received: by mail-qk0-f181.google.com with SMTP id v127so142244593qkb.2 for ; Tue, 21 Mar 2017 11:44:50 -0700 (PDT) Date: Tue, 21 Mar 2017 11:44:01 -0700 From: Omar Sandoval To: Linus Torvalds Cc: Al Viro , linux-fsdevel , Linux API , kernel-team , Xi Wang , Omar Sandoval Subject: Re: [RFC PATCH v2 1/2] fs: add AT_REPLACE flag for linkat() which replaces the target Message-ID: <20170321184401.GA32507@vader> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Mar 21, 2017 at 09:30:30AM -0700, Linus Torvalds wrote: > On Tue, Mar 21, 2017 at 7:51 AM, Omar Sandoval wrote: > > */ > > -int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry, struct inode **delegated_inode) > > +int vfs_link(struct dentry *old_dentry, struct inode *dir, > > + struct dentry *new_dentry, struct inode **delegated_inode, > > + unsigned int flags) > > { > > struct inode *inode = old_dentry->d_inode; > > + struct inode *target = new_dentry->d_inode; > > unsigned max_links = dir->i_sb->s_max_links; > > int error; > > > > if (!inode) > > return -ENOENT; > > > > - error = may_create(dir, new_dentry); > > + if (target) { > > + if (flags & AT_REPLACE) > > + error = may_delete(dir, new_dentry, d_is_dir(old_dentry)); > > + else > > + error = -EEXIST; > > + } else { > > + error = may_create(dir, new_dentry); > > + } Hi, Linus, Thanks for taking a look. > This looks bogus. > > In particular, that "may_delete()" cannot be right. It should still > *also* have the right to create something in that directory. > > But even if you replace it with checks for both deletion _and_ > creation, it won't be right, since the test for d_is_negative() on the > target in may_delete() looks wrong for this situation. > > Normally, you cannot delete a negative entry (think of what that would > do in a overlay situation), but it should still be ok to link a > positive entry on top of a negative one. This is actually the exact same check we have in vfs_rename(): ---- if (!target) { error = may_create(new_dir, new_dentry); } else { new_is_dir = d_is_dir(new_dentry); if (!(flags & RENAME_EXCHANGE)) error = may_delete(new_dir, new_dentry, is_dir); else error = may_delete(new_dir, new_dentry, new_is_dir); } ---- I tried to keep the same semantics as rename(). > Also, I think the above is incorrect for yet another case: moving > somethign on top of a directory should be disallowed. We do later on > check that the *source* isn't a directory (since you can't link > directories), but I'm not seeing where you'd be checking that you > don't move over a directory either. That check is happening in may_delete(): ---- if (isdir) { if (!d_is_dir(victim)) return -ENOTDIR; if (IS_ROOT(victim)) return -EBUSY; } else if (d_is_dir(victim)) return -EISDIR; ---- > So I don't think this patch is acceptable as such. I also suspect that > it should be done in multiple phases. > > Linus