From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Date: Tue, 09 May 2017 17:27:03 -0500 Message-ID: <87mvalu0nc.fsf@xmission.com> References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> <87a86mvuko.fsf@xmission.com> <20170509203736.GB14900@mail.hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170509203736.GB14900-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org> (Serge E. Hallyn's message of "Tue, 9 May 2017 15:37:36 -0500") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: "Serge E. Hallyn" Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, lkp-JC7UmRfGjtg@public.gmane.org, xiaolong.ye-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, LKML List-Id: containers.vger.kernel.org "Serge E. Hallyn" writes: > Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org): >> "Serge E. Hallyn" writes: >> > Changelog: >> [snip] >> > May 8, 2017: >> > . fix leaking dentry refcount in cap_inode_getsecurity >> > >> [snip] >> > +/* >> > + * getsecurity: We are called for security.* before any attempt to read the >> > + * xattr from the inode itself. >> > + * >> > + * This gives us a chance to read the on-disk value and convert it. If we >> > + * return -EOPNOTSUPP, then vfs_getxattr() will call the i_op handler. >> > + * >> > + * Note we are not called by vfs_getxattr_alloc(), but that is only called >> > + * by the integrity subsystem, which really wants the unconverted values - >> > + * so that's good. >> > + */ >> > +int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer, >> > + bool alloc) >> > +{ >> > + int size, ret; >> > + kuid_t kroot; >> > + uid_t root, mappedroot; >> > + char *tmpbuf = NULL; >> > + struct vfs_cap_data *cap; >> > + struct vfs_ns_cap_data *nscap; >> > + struct dentry *dentry; >> > + struct user_namespace *fs_ns; >> > + >> > + if (strcmp(name, "capability") != 0) >> > + return -EOPNOTSUPP; >> > + >> > + dentry = d_find_alias(inode); >> > + if (!dentry) >> > + return -EINVAL; >> > + >> > + size = sizeof(struct vfs_ns_cap_data); >> > + ret = (int) vfs_getxattr_alloc(dentry, XATTR_NAME_CAPS, >> > + &tmpbuf, size, GFP_NOFS); >> > + dput(dentry); >> >> This looks like a good fix but ouch! That interface is wrong. >> >> The dentry is needed because vfs_getxattr_alloc does: >> error = handler->get(handler, dentry, inode, name, NULL, 0); >> >> Which is has no business taking a dentry as xattrs are inode concepts. >> >> I have no issue with your patch but it looks like that handler issue >> is going to need to be fixed with xattrs. > > True, it's a bit clunky. > > Any reason not to just have the current vfs_getxattr_alloc() become a > lightweight wrapper calling inode_getxattr_alloc(dentry->d_inode)? My deep issue is that handler is functions like posix_acl_xattr_get. And all of those functions that vfs_getxattr_alloc calls should not take a dentry. So I feel like I have just spotted the tip of an iceberg that needs sorting out. Eric