Re: struct ifnet and ifaddr handling [was: Re: Making global variables of if.c MPSAFE]

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Dec 1, 2014 at 7:59 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> Hi,
>
> I found an defect of ifnet object initializations.
>
> - if_attach and if_alloc_sadl is called in each
>   interface XXXattach function
>   - if_alloc_sadl may be called via XXX_ifattach
>     (e.g., ether_ifattach)
> - if_attach initializes an ifnet object, but
>   the initialization is incomplete and if_alloc_sadl
>   does the rest
>   - for example, ifp->if_dl and ifp->if_sadl
> - if_attach enables entering ioctl (doifioctl)
> - ioctl can be called between if_attach and
>   if_alloc_sadl
> - ioctl may touch uninitialized member values
>   of ifnet
>   - then boom
> - sysctl also has the same issue
>
> We have to prevent ioctl from being called until
> if_alloc_sadl is finished somehow. We can achieve
> the goal by postponing enabling ioctl or registering
> an ifnet object to ifnet_list until if_alloc_sadl
> is finished. To this end, we need to call something
> after if_alloc_sadl.
>
> My approach is to split if_attach into if_init and
> if_attach; put initializations of if_attach into
> if_init and put registrations (e.g., adding a ifnet
> object to ifnet_list and sysctl setups) into if_attach.
> Then, call new if_attach after if_alloc_sadl so that
> we can ensure ioctl and sysctl are called after all
> initializations done.
>
> [before]
>   if_attach();
>   if_alloc_sadl();
>
> [after]
>   if_init();
>   if_alloc_sadl();
>   if_attach();
>
> A concern of the approach is that it requires
> modifications to every places where if_attach
> is used (i.e., if_*).
>
> Here is a patch: http://www.netbsd.org/~ozaki-r/if_attach.diff
> (This patch includes if_attach modifications of only some if_*,
> not all, yet.)
>
> Any comments?

I've changed my mind. Changing every drivers isn't feasible at this
point. So I keep if_attach and add new two API, if_initialize
and if_register. if_attach just calls the two so that users of
if_attach works as it used to be. I'll change only some drivers that
I need, to use the new APIs. That's enough to me for now.

So here is a new patch: http://www.netbsd.org/~ozaki-r/if_attach.diff

  ozaki-r