Re: [PATCH -mm] sys_unshare: simplify the not-really-implemented CLONE_THREAD/SIGHAND/VM code

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.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