On Fri, Jul 10, 2020 at 01:22:40PM +0200, Martin Pieuchot wrote:
> On 10/07/20(Fri) 14:07, Vitaliy Makkoveev wrote:
> > We have some races in pppac(4)
> > 1. malloc(9) can sleep so we must check `sc' presence after malloc(9)
>
> Makes sense.
>
> > 2. we can sleep between `sc' insertion to `sc_entry' list and
> > `sc_pipex_iface' initialization. Concurrent pppacioctl() can touch
> > this incomplete `sc'.
>
> Why not insert the descriptor at the end? Shouldn't the order of
> operations be:
>
> pipex_iface_init();
> if_attach();
> LIST_INSERT_HEAD()
>
> This way there's no need for a `ready' flag since the descriptor is only
> added to global data structures once it is completely initialized.
>
> Using a `sc_ready' or `sc_dead' approach is something that require
> touching all drivers whereas serializing insertions to global data
> structures can be done at once for all the kernel.
No, because we introduce the races with if_attach(). The similar races
are in if_clone_attach(). We can do multiple `ifp' attachment with the
same name.
---- cut begin ----
int
pppacopen(dev_t dev, int flags, int mode, struct proc *p)
{
sc = malloc(sizeof(*sc), M_DEVBUF, M_WAITOK|M_ZERO);
ifp = &sc->sc_if;
snprintf(ifp->if_xname, sizeof(ifp->if_xname), "pppac%u",
minor(dev));
pipex_iface_init(); /* XXX: can sleep */
if_attach(); /* XXX: can sleep */
LIST_INSERT_HEAD();
}
---- cut end ----
So we insert incomplete `sc' with used `dev' before context switch.
Or if_attach() should check passed `ifp'.
