Re: [PATCH 01/23] tref: Implement task references.

[Oleg Nesterov <oleg@tv-sign.ru>]

"Eric W. Biederman" wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> 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.