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 11:55:19 -0500 Message-ID: <87a86mvuko.fsf@xmission.com> References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170508181156.GA23112-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org> (Serge E. Hallyn's message of "Mon, 8 May 2017 13:11:56 -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: > 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. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756050AbdEIRBt (ORCPT ); Tue, 9 May 2017 13:01:49 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:60893 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755818AbdEIRBq (ORCPT ); Tue, 9 May 2017 13:01:46 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: "Serge E. Hallyn" Cc: Masami Ichikawa , containers@lists.linux-foundation.org, lkp@01.org, xiaolong.ye@intel.com, LKML References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> Date: Tue, 09 May 2017 11:55:19 -0500 In-Reply-To: <20170508181156.GA23112@mail.hallyn.com> (Serge E. Hallyn's message of "Mon, 8 May 2017 13:11:56 -0500") Message-ID: <87a86mvuko.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1d88W2-00033z-Q8;;;mid=<87a86mvuko.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18uvo22ogaD8mJrYTXfswKt2bMLwvOU4Q8= X-SA-Exim-Connect-IP: 97.121.81.159 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.4961] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;"Serge E. Hallyn" X-Spam-Relay-Country: X-Spam-Timing: total 5305 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 2.6 (0.0%), b_tie_ro: 1.67 (0.0%), parse: 1.37 (0.0%), extract_message_metadata: 14 (0.3%), get_uri_detail_list: 1.82 (0.0%), tests_pri_-1000: 6 (0.1%), tests_pri_-950: 1.75 (0.0%), tests_pri_-900: 1.47 (0.0%), tests_pri_-400: 38 (0.7%), check_bayes: 37 (0.7%), b_tokenize: 10 (0.2%), b_tok_get_all: 6 (0.1%), b_comp_prob: 3.3 (0.1%), b_tok_touch_all: 6 (0.1%), b_finish: 4.2 (0.1%), tests_pri_0: 357 (6.7%), check_dkim_signature: 0.61 (0.0%), check_dkim_adsp: 3.1 (0.1%), tests_pri_500: 4878 (91.9%), poll_dns_idle: 4872 (91.8%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============5129842081440866776==" MIME-Version: 1.0 From: Eric W. Biederman To: lkp@lists.01.org Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Date: Tue, 09 May 2017 11:55:19 -0500 Message-ID: <87a86mvuko.fsf@xmission.com> In-Reply-To: <20170508181156.GA23112@mail.hallyn.com> List-Id: --===============5129842081440866776== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable "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 call= ed > + * 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 =3D NULL; > + struct vfs_cap_data *cap; > + struct vfs_ns_cap_data *nscap; > + struct dentry *dentry; > + struct user_namespace *fs_ns; > + > + if (strcmp(name, "capability") !=3D 0) > + return -EOPNOTSUPP; > + > + dentry =3D d_find_alias(inode); > + if (!dentry) > + return -EINVAL; > + > + size =3D sizeof(struct vfs_ns_cap_data); > + ret =3D (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 =3D 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. Eric --===============5129842081440866776==--