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 */
>
