diff mbox

fs: remove excess check for in_userns

Message ID 1458043740-14229-1-git-send-email-ptikhomirov@virtuozzo.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Tikhomirov March 15, 2016, 12:09 p.m. UTC
If in_userns returns false mnt_may_suid also returns false, and we
will reach second(removed) if-check only in case it does not trigger,
so remove it.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 security/commoncap.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Seth Forshee March 15, 2016, 1:45 p.m. UTC | #1
On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote:
> If in_userns returns false mnt_may_suid also returns false, and we
> will reach second(removed) if-check only in case it does not trigger,
> so remove it.

We had a somewhat lengthy discussion previously where one of the
conclusions was that we'd have that check in both places even though
it's redundant. Iirc the reason was that though they're doing the same
test they're doing so to answer different questions, so we should have
the test in both places (or something along those lines).

Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Tikhomirov March 15, 2016, 2:19 p.m. UTC | #2
On 03/15/2016 04:45 PM, Seth Forshee wrote:
> On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote:
>> If in_userns returns false mnt_may_suid also returns false, and we
>> will reach second(removed) if-check only in case it does not trigger,
>> so remove it.
>
> We had a somewhat lengthy discussion previously where one of the
> conclusions was that we'd have that check in both places even though
> it's redundant. Iirc the reason was that though they're doing the same
> test they're doing so to answer different questions, so we should have
> the test in both places (or something along those lines).

Ok, that is reasonable. But from my POW the edge between the meaning of 
those checks is quiet blurred.

Thanks!

>
> Thanks,
> Seth
>
Pavel Tikhomirov March 15, 2016, 2:19 p.m. UTC | #3
On 03/15/2016 04:45 PM, Seth Forshee wrote:
> On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote:
>> If in_userns returns false mnt_may_suid also returns false, and we
>> will reach second(removed) if-check only in case it does not trigger,
>> so remove it.
>
> We had a somewhat lengthy discussion previously where one of the
> conclusions was that we'd have that check in both places even though
> it's redundant. Iirc the reason was that though they're doing the same
> test they're doing so to answer different questions, so we should have
> the test in both places (or something along those lines).

Ok, that is reasonable. But from my POV the edge between the meaning of 
those checks is quiet blurred.

Thanks!

>
> Thanks,
> Seth
>
James Morris March 22, 2016, 11:19 p.m. UTC | #4
On Tue, 15 Mar 2016, Seth Forshee wrote:

> On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote:
> > If in_userns returns false mnt_may_suid also returns false, and we
> > will reach second(removed) if-check only in case it does not trigger,
> > so remove it.
> 
> We had a somewhat lengthy discussion previously where one of the
> conclusions was that we'd have that check in both places even though
> it's redundant. Iirc the reason was that though they're doing the same
> test they're doing so to answer different questions, so we should have
> the test in both places (or something along those lines).

A comment in the code might be useful here.
diff mbox

Patch

diff --git a/security/commoncap.c b/security/commoncap.c
index ca0c04ae..82f930c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -445,8 +445,6 @@  static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 
 	if (!mnt_may_suid(bprm->file->f_path.mnt))
 		return 0;
-	if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns))
-		return 0;
 
 	dentry = dget(bprm->file->f_dentry);