Messages in this thread | | | Subject | Re: [PATCH -mm] sys_unshare: simplify the not-really-implemented CLONE_THREAD/SIGHAND/VM code | From | (Eric W. Biederman) | Date | Tue, 23 Mar 2010 14:02:55 -0700 |
| |
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
| |