On 13/09/20(Sun) 10:05, Klemens Nanni wrote: > > Here's a start at struct pppoe_softc; for every member I went through > code paths looking for *_LOCK() or *_ASSERT_LOCKED(). > > Pretty much all members are under the net lock, some are proctected by > both net and kernel lock, e.g. the start routine is called with > KERNEL_LOCK(). > > I did not go through the sppp struct members yet. > > Does this look correct?
Without doing another audit but with the fact that pseudo-device are generally run by a thread holding the NET_LOCK() I'd assume it's ok. > From here on, I think we can start and already pull out a few of those > members and put them under a new pppoe(4) specific lock. First we should remove the KRENEL_LOCK() from around pppoeintr(). The NET_LOCK() is not an issue right now. One comment below, either way ok mpi@ > Index: if_pppoe.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppoe.c,v > retrieving revision 1.70 > diff -u -p -r1.70 if_pppoe.c > --- if_pppoe.c 28 Jul 2020 09:52:32 -0000 1.70 > +++ if_pppoe.c 20 Aug 2020 15:27:09 -0000 > @@ -114,27 +115,34 @@ struct pppoetag { > #define PPPOE_DISC_MAXPADI 4 /* retry PADI four times > (quickly) */ > #define PPPOE_DISC_MAXPADR 2 /* retry PADR twice */ > > +/* > + * Locks used to protect struct members and global data > + * I immutable after creation > + * K kernel lock I wouldn't bother repeating 'I' and 'K' if they are not used in the description below. > + * N net lock > + */ > + > struct pppoe_softc { > struct sppp sc_sppp; /* contains a struct ifnet as first > element */ > - LIST_ENTRY(pppoe_softc) sc_list; > - unsigned int sc_eth_ifidx; > + LIST_ENTRY(pppoe_softc) sc_list;/* [N] */ > + unsigned int sc_eth_ifidx; /* [N] */ > > - int sc_state; /* discovery phase or session connected > */ > - struct ether_addr sc_dest; /* hardware address of concentrator */ > - u_int16_t sc_session; /* PPPoE session id */ > - > - char *sc_service_name; /* if != NULL: requested name of > service */ > - char *sc_concentrator_name; /* if != NULL: requested concentrator > id */ > - u_int8_t *sc_ac_cookie; /* content of AC cookie we must echo > back */ > - size_t sc_ac_cookie_len; /* length of cookie data */ > - u_int8_t *sc_relay_sid; /* content of relay SID we must echo > back */ > - size_t sc_relay_sid_len; /* length of relay SID data */ > - u_int32_t sc_unique; /* our unique id */ > - struct timeout sc_timeout; /* timeout while not in session state */ > - int sc_padi_retried; /* number of PADI retries already done > */ > - int sc_padr_retried; /* number of PADR retries already done > */ > + int sc_state; /* [N] discovery phase or session > connected */ > + struct ether_addr sc_dest; /* [N] hardware address of concentrator > */ > + u_int16_t sc_session; /* [N] PPPoE session id */ > + > + char *sc_service_name; /* [N] if != NULL: requested name of > service */ > + char *sc_concentrator_name; /* [N] if != NULL: requested > concentrator id */ > + u_int8_t *sc_ac_cookie; /* [N] content of AC cookie we must > echo back */ > + size_t sc_ac_cookie_len; /* [N] length of cookie data */ > + u_int8_t *sc_relay_sid; /* [N] content of relay SID we must > echo back */ > + size_t sc_relay_sid_len; /* [N] length of relay SID data */ > + u_int32_t sc_unique; /* [I] our unique id */ > + struct timeout sc_timeout; /* [N] timeout while not in session > state */ > + int sc_padi_retried; /* [N] number of PADI retries already > done */ > + int sc_padr_retried; /* [N] number of PADR retries already > done */ > > - struct timeval sc_session_time; /* time the session was established */ > + struct timeval sc_session_time; /* [N] time the session was established > */ > }; > > /* incoming traffic will be queued here */ >