linux-kernel - Re: [PATCH -mm] sys_unshare: simplify the not-really-implemented CLONE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m1hbo6u4xs.fsf@fess.ebiederm.org>
Date:	Tue, 23 Mar 2010 14:02:55 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Badari Pulavarty <pbadari@...ibm.com>,
	Christoph Hellwig <hch@....de>, Janak Desai <janak@...ibm.com>,
	Roland McGrath <roland@...hat.com>,
	Stanislaw Gruszka <sgruszka@...hat.com>,
	Sukadev Bhattiprolu <sukadev@...ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH -mm] sys_unshare: simplify the not-really-implemented CLONE_THREAD/SIGHAND/VM code

Oleg Nesterov <oleg@...hat.com> writes:

> (on top of check_unshare_flags-kill-the-bogus-clone_sighand-sig-count-check.patch)
>
> Cleanup.
>
> sys_unshare(CLONE_THREAD/SIGHAND/VM) is not really implemented, and I doubt
> very much it will ever work. At least, nobody even tried since the original
> "unshare system call -v5: system call handler function" commit
> 99d1419d96d7df9cfa56bc977810be831bd5ef64 was applied more than 4 years ago.
>
> And the code is not consistent. unshare_thread() always fails unconditionally,
> while unshare_sighand() and unshare_vm() pretend to work if there is nothing
> to unshare.

This is setting off alarm bells in my head.

I haven't traced this all through but I like your logic a lot less, and
I think it is buggy.  Why don't we need to look at sigh->count ?

The current logic is very fine grained but it does a lot of simple logical
checks and it ties those checks together if a very maintainable way.

You require that we know upfront all of the dependencies, which is things
change subtlety can be a maintenance challenge.



> Remove unshare_thread(), unshare_sighand(), unshare_vm() helpers and related
> variables and add a simple CLONE_THREAD | CLONE_SIGHAND| CLONE_VM check into
> check_unshare_flags().
>
> Also, move the "CLONE_NEWNS needs CLONE_FS" check from check_unshare_flags()
> to sys_unshare(). This looks more consistent and matches the similar
> do_sysvsem check in sys_unshare().
>
> Note: with or without this patch "atomic_read(mm->mm_users) > 1" can give
> a false positive due to get_task_mm().

I think the number of times get_task_mm is called on not current this isn't
an interesting race.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/