From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ram Pai Subject: Re: Question regarding shared subtree mount flags. Date: Wed, 29 Feb 2012 13:59:49 +0800 Message-ID: <20120229055949.GA5030@ram-ThinkPad-T61> References: <201202282233.EJF00063.VLFOSFMtJQOHFO@I-love.SAKURA.ne.jp> Reply-To: Ram Pai Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org To: Tetsuo Handa Return-path: Received: from e1.ny.us.ibm.com ([32.97.182.141]:46145 "EHLO e1.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751603Ab2B2GAI (ORCPT ); Wed, 29 Feb 2012 01:00:08 -0500 Received: from /spool/local by e1.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 29 Feb 2012 01:00:06 -0500 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 97A036E8049 for ; Wed, 29 Feb 2012 00:59:53 -0500 (EST) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q1T5xqg4303510 for ; Wed, 29 Feb 2012 00:59:53 -0500 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q1T5xqPV031745 for ; Tue, 28 Feb 2012 22:59:52 -0700 Content-Disposition: inline In-Reply-To: <201202282233.EJF00063.VLFOSFMtJQOHFO@I-love.SAKURA.ne.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Feb 28, 2012 at 10:33:15PM +0900, Tetsuo Handa wrote: > I noticed that TOMOYO's mount flag checking priority > > if (flags & MS_REMOUNT) { > type = tomoyo_mounts[TOMOYO_MOUNT_REMOUNT]; > flags &= ~MS_REMOUNT; > } > if (flags & MS_MOVE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MOVE]; > flags &= ~MS_MOVE; > } > if (flags & MS_BIND) { > type = tomoyo_mounts[TOMOYO_MOUNT_BIND]; > flags &= ~MS_BIND; > } > if (flags & MS_UNBINDABLE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_UNBINDABLE]; > flags &= ~MS_UNBINDABLE; > } > if (flags & MS_PRIVATE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_PRIVATE]; > flags &= ~MS_PRIVATE; > } > if (flags & MS_SLAVE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SLAVE]; > flags &= ~MS_SLAVE; > } > if (flags & MS_SHARED) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SHARED]; > flags &= ~MS_SHARED; > } > if (!type) > type = ""; > idx = tomoyo_read_lock(); > error = tomoyo_mount_acl(&r, dev_name, path, type, flags); > tomoyo_read_unlock(idx); > > is wrong and it need to follow priority used by do_mount(). > > if (flags & MS_REMOUNT) > retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags, > data_page); > else if (flags & MS_BIND) > retval = do_loopback(&path, dev_name, flags & MS_REC); > else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > retval = do_change_type(&path, flags); > else if (flags & MS_MOVE) > retval = do_move_mount(&path, dev_name); > > I got a question regarding priority among MS_SHARED, MS_PRIVATE, MS_SLAVE and > MS_UNBINDABLE. I'm wondering which flag TOMOYO should use for permission check > if more than one of MS_SHARED, MS_PRIVATE, MS_SLAVE or MS_UNBINDABLE is passed. > > commit 07b20889 "[PATCH] beginning of the shared-subtree proper" > commit 03e06e68 "[PATCH] introduce shared mounts" > commit a58b0eb8 "[PATCH] introduce slave mounts" > commit 9676f0c6 "[PATCH] unbindable mounts" > > introduced these flags and > > commit 7a2e8a8f "VFS: Sanity check mount flags passed to change_mnt_propagation()" > > clarified that these flags must be exclusively passed. right. It makes more sense for a user to change a vfsmount to one type at a given time. > > Commit 7a2e8a8f went to 2.6.36 but has not gone to 2.6.27 nor 2.6.32. > Is this simply because commit 7a2e8a8f was not proposed for backporting? looks like it. No one saw the need to backport that patch. > > If so, is security_sb_mount() allowed to return -EINVAL if more than one of > these flags are passed (like shown below) so that TOMOYO will not generate > inaccurate audit logs? > > if (flags & MS_REMOUNT) { > type = tomoyo_mounts[TOMOYO_MOUNT_REMOUNT]; > flags &= ~MS_REMOUNT; > } else if (flags & MS_BIND) { > type = tomoyo_mounts[TOMOYO_MOUNT_BIND]; > flags &= ~MS_BIND; > } else if (flags & MS_SHARED) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SHARED]; > flags &= ~MS_SHARED; > if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > return -EINVAL; > } else if (flags & MS_PRIVATE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_PRIVATE]; > flags &= ~MS_PRIVATE; > if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > return -EINVAL; > } else if (flags & MS_SLAVE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_SLAVE]; > flags &= ~MS_SLAVE; > if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > return -EINVAL; > } else if (flags & MS_UNBINDABLE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MAKE_UNBINDABLE]; > flags &= ~MS_UNBINDABLE; > if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > return -EINVAL; > } else if (flags & MS_MOVE) { > type = tomoyo_mounts[TOMOYO_MOUNT_MOVE]; > flags &= ~MS_MOVE; > } I think that is right. However the code can be further reorganized to check for (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE) only ones, instead of checking under every else if. RP > if (!type) > type = ""; > idx = tomoyo_read_lock(); > error = tomoyo_mount_acl(&r, dev_name, path, type, flags); > tomoyo_read_unlock(idx);