From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Date: Tue, 9 May 2017 15:37:36 -0500 Message-ID: <20170509203736.GB14900@mail.hallyn.com> References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> <87a86mvuko.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <87a86mvuko.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 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: "Eric W. Biederman" Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, LKML , xiaolong.ye-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, lkp-JC7UmRfGjtg@public.gmane.org, "Serge E. Hallyn" List-Id: containers.vger.kernel.org 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)? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753066AbdEIUh3 (ORCPT ); Tue, 9 May 2017 16:37:29 -0400 Received: from h2.hallyn.com ([78.46.35.8]:47376 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751087AbdEIUh2 (ORCPT ); Tue, 9 May 2017 16:37:28 -0400 Date: Tue, 9 May 2017 15:37:36 -0500 From: "Serge E. Hallyn" To: "Eric W. Biederman" Cc: "Serge E. Hallyn" , Masami Ichikawa , containers@lists.linux-foundation.org, lkp@01.org, xiaolong.ye@intel.com, LKML Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Message-ID: <20170509203736.GB14900@mail.hallyn.com> References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> <87a86mvuko.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a86mvuko.fsf@xmission.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Eric W. Biederman (ebiederm@xmission.com): > "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)?