svn commit: r228969 - head/sys/netinet

[Marko Zec]

On Thursday 17 October 2013 21:35:46 Mikolaj Golub wrote:
> On Wed, Oct 16, 2013 at 05:09:04PM -0400, John Baldwin wrote:
> ...
>
> > >  >> #10 0x80b6f1fd in igmp_change_state (inm=0x8ae70480)
> > >  >>     at /home/golub/freebsd/base/head/sys/netinet/igmp.c:2595
> > >  >> #11 0x80b76f68 in in_leavegroup_locked (inm=0x8ae70480,
> > >  >> imf=0x8a655a00) at
> > >  >> /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1239 #12
> > >  >> 0x80b76fbd in in_leavegroup (inm=0x8ae70480, imf=0x8a655a00) at
> > >  >> /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1184 #13
> > >  >> 0x80b770b4 in inp_gcmoptions (context=0x0, pending=1) at
> > >  >> /home/golub/freebsd/base/head/sys/netinet/in_mcast.c:1554 #14
> > >  >> 0x80a8ff2b in taskqueue_run_locked (queue=0x87594880) at
> > >  >> /home/golub/freebsd/base/head/sys/kern/subr_taskqueue.c:308 #15
> > >  >> 0x80a90987 in taskqueue_thread_loop (arg=0x81186bcc) at
> > >  >> /home/golub/freebsd/base/head/sys/kern/subr_taskqueue.c:497 #16
> > >  >> 0x80a1b2d8 in fork_exit (callout=0x80a90920
> > >  >> <taskqueue_thread_loop>, arg=0x81186bcc,
> > >  >>     frame=0x872b2d28) at
> > >  >> /home/golub/freebsd/base/head/sys/kern/kern_fork.c:992
>
> ...
>
> > >  >> VNET context is not set at that point.
>
> ...
>
> > I think this was just fixed by glebius@ in r256587:
> >
> > Author: glebius
> > Date: Wed Oct 16 05:02:01 2013
> > New Revision: 256587
> > URL: http://svnweb.freebsd.org/changeset/base/256587
> >
> > Log:
> >   For VIMAGE kernels store vnet in the struct task, and set vnet
> > context during task processing.
> >
> >   Reported & tested by: mm
>
> I think that particular issue was fixed earlier by hrs in r252510:
>
>   Fix a panic when leaving MC group in a kernel with VIMAGE enabled.
>   in_leavegroup() is called from an asynchronous task, and
>   igmp_change_state() requires that curvnet is set by the caller.
>
> Concerning this more general solution from Gleb, with storing the vnet
> pointer in the task, I wander how it is safe when the vnet is being
> removed?
>
> In vnet_destroy() the vnet interfaces are moved back to their parent
> vnets, so setting the vnet context from the interface looks safer.

+1

Gleb,

what exactly was the justification for embedding a vnet context inside 
struct task, when it could have been easily embedded, directly or 
indirectly, inside the object pointed to by ta_context?

What is / was your plan to garbage collect such stale pointers?

At least in the early days of VIMAGE / VNET hacking, all the people involved 
strived very hard to store backpointers to vnets in as few structs as 
possible, and for quite a while we managed to constrain this to only ifnets 
and sockets, and both of those references are refcounted in struct vnet.

Now, more (unreferenced) backpointers to struct vnet seem to be popping up 
here and there, but in most cases they are embedded in objects which are 
guaranteed to persist only as long as the referenced vnet is alive.  One 
notable exception to this rule is struct tcpcb which apparently can outlive 
vnet teardowns under certain conditions, but this is a known issue which 
needs to be fixed, not a precedent which calls for more timebombs to be 
scattered around the source tree.

So pls. reconsider solving the issue at hand using a different approach, and 
back out r256587.

Thanks,

Marko