"Eric W. Biederman" wrote:
>
> Oleg Nesterov <[email protected]> writes:
>
> > fastcall void free_pidmap(int pid)
> > {
> > pidmap_t *map = pidmap_array + pid / BITS_PER_PAGE;
> > int offset = pid & BITS_PER_PAGE_MASK;
> > struct pid_ref *ref;
> >
> > clear_bit(offset, map->page);
> > atomic_inc(&map->nr_free);
> >
> > ref = find_pid_ref(pid);
> > if (unlikely(ref != NULL)) {
> > hlist_del_init(&ref->chain);
> > ref->pid = 0;
> > }
> > }
>
> Ouch! I believe free_pidmap now needs the tasklist_lock so
> we can free the pid and kill the pid_ref atomically. Otherwise
> the pid could potentially get reused before we free the pid reference.
> I think that means ensuring all of the callers take tasklist_lock.
Yes, you are right. And do_fork() does free_pidmap() lockless in
the error path. This path is not performance critical, so may be
it is ok to add wrie_lock(tasklist) here.
> > void free_pid_ref(struct pid_ref *ref)
> > {
> > if (!ref)
> > return;
> >
> > write_lock_irq(&tasklist_lock);
> > if (!--ref->count) {
> > hlist_del_init(&ref->chain);
> > kfree(ref);
> > }
> > write_unlock_irq(&tasklist_lock);
> > }
>
> I think calling this put_pid_ref instead of free_pid_ref
> is more accurate. The whole alloc/free _pid_ref instead
> of the more traditional get/put kind of throws me. Since
> an allocation/free is possible I can see where this comes from
> but I don't feel right about those names.
Agree.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
