svn commit: r184762 - head/sys/netgraph

Attilio Rao attilio at freebsd.org
Sat Nov 8 06:47:43 PST 2008


2008/11/8, Kostik Belousov <kostikbel at gmail.com>:
> On Sat, Nov 08, 2008 at 02:13:46PM +0100, Attilio Rao wrote:
>  > 2008/11/8, Alexander Motin <mav at freebsd.org>:
>  > > Author: mav
>  > >  Date: Sat Nov  8 06:25:57 2008
>  > >  New Revision: 184762
>  > >  URL: http://svn.freebsd.org/changeset/base/184762
>  > >
>  > >  Log:
>  > >   Don't use curthread to resolve file descriptor. Request may be queued, so
>  > >   thread will be different. Instead require sender to send process ID
>  > >   together with file descriptor.
>  > >
>  > >  Modified:
>  > >   head/sys/netgraph/ng_tty.c
>  > >
>  > >  Modified: head/sys/netgraph/ng_tty.c
>  > >  ==============================================================================
>  > >  --- head/sys/netgraph/ng_tty.c  Sat Nov  8 04:43:24 2008        (r184761)
>  > >  +++ head/sys/netgraph/ng_tty.c  Sat Nov  8 06:25:57 2008        (r184762)
>  > >  @@ -73,6 +73,7 @@
>  > >   #include <sys/syslog.h>
>  > >   #include <sys/tty.h>
>  > >   #include <sys/ttycom.h>
>  > >  +#include <sys/proc.h>
>  > >
>  > >   #include <net/if.h>
>  > >   #include <net/if_var.h>
>  > >  @@ -250,7 +251,8 @@ ngt_shutdown(node_p node)
>  > >   static int
>  > >   ngt_rcvmsg(node_p node, item_p item, hook_p lasthook)
>  > >   {
>  > >  -       struct thread *td = curthread;  /* XXX */
>  > >  +       struct proc *p;
>  > >  +       struct thread *td;
>  > >         const sc_p sc = NG_NODE_PRIVATE(node);
>  > >         struct ng_mesg *msg, *resp = NULL;
>  > >         int error = 0;
>  > >  @@ -262,8 +264,14 @@ ngt_rcvmsg(node_p node, item_p item, hoo
>  > >                 case NGM_TTY_SET_TTY:
>  > >                         if (sc->tp != NULL)
>  > >                                 return (EBUSY);
>  > >  -                       error = ttyhook_register(&sc->tp, td, *(int *)msg->data,
>  > >  +
>  > >  +                       p = pfind(((int *)msg->data)[0]);
>  > >  +                       if (p == NULL)
>  > >  +                               return (ESRCH);
>  > >  +                       td = FIRST_THREAD_IN_PROC(p);
>  > >  +                       error = ttyhook_register(&sc->tp, td, ((int *)msg->data)[1],
>  > >                             &ngt_hook, sc);
>  > >  +                       PROC_UNLOCK(p);
>  > >                         if (error != 0)
>  > >                                 return (error);
>  > >                         break;
>  >
>  > The threads iterator in strcut proc should be proc_slock protected, so
>  > you need to grab/release it around FIRST_THREAD_IN_PROC().
>
>
> I do not believe that process slock is needed there, since code
>  just dereference a pointer. The process lock seems to be taken to
>  guarantee that td is alive.
>
>  On the other hand, ttyhook_register locks filedesc sx, that causes
>  sleepable lock acquition after non-sleepable. I believe this is
>  an actual problem with the change.
>
>  Probably, this is even the problem with KPI of ttyhook_register, since
>  caller need to somehow protect td from going away, and proc lock is
>  a right lock to held.

I spoke briefly with Kostik on IRC and the problem he exposes is real.
He suggested to use directly a struct file object to be passed and it
seems fine by me.

Also, as you are not interested in what thread is returned as first,
you probabilly don't need the proc_slock.

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein


More information about the svn-src-all mailing list