On Wed, Jun 22, 2022 at 02:46:32PM +0300, Vitaliy Makkoveev wrote:
> @@ -547,9 +547,11 @@ ether_input(struct ifnet *ifp, struct mb
>
> if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
> pipex_pppoe_input(m, session);
> + pipex_rele_session(session);
> KERNEL_UNLOCK();
> return;
> }
> + pipex_rele_session(session);
> }
> #endif
> if (etype == ETHERTYPE_PPPOEDISC)
Within pipex_pppoe_lookup_session() we have this code:
struct pipex_session *
pipex_pppoe_lookup_session(struct mbuf *m0)
{
...
session = pipex_lookup_by_session_id(PIPEX_PROTO_PPPOE,
pppoe.session_id);
...
if (session && session->proto.pppoe.over_ifidx != m0->m_pkthdr.ph_ifidx)
session = NULL;
return (session);
}
You have to call pipex_rele_session() before setting session to NULL.
> @@ -152,24 +158,26 @@ struct cpumem;
> /* pppac ip-extension session table */
> struct pipex_session {
> struct radix_node ps4_rn[2];
> - /* [N] tree glue, and other values */
> + /* [L] tree glue, and other values */
> struct radix_node ps6_rn[2];
> - /* [N] tree glue, and other values */
> + /* [L] tree glue, and other values */
> +
> + struct refcnt pxs_refs;
Can we call this pxs_refcnt? That is what most of the other code
calls this field. Although it is already inconsistent.
> struct mutex pxs_mtx;
>
> - LIST_ENTRY(pipex_session) session_list; /* [N] all session chain */
> - LIST_ENTRY(pipex_session) state_list; /* [N] state list chain */
> - LIST_ENTRY(pipex_session) id_chain; /* [N] id hash chain */
> + LIST_ENTRY(pipex_session) session_list; /* [L] all session chain */
> + LIST_ENTRY(pipex_session) state_list; /* [L] state list chain */
> + LIST_ENTRY(pipex_session) id_chain; /* [L] id hash chain */
> LIST_ENTRY(pipex_session) peer_addr_chain;
> - /* [N] peer's address hash chain */
> - uint16_t state; /* [N] pipex session state */
> + /* [L] peer's address hash chain */
> + uint16_t state; /* [L] pipex session state */
Can we use a u_int value here? We want to mark it MP safe. Should
be no difference in binary as the previous and next field are 32
bit aligned. Or do we rely on the fact that both are also [L]
protected?
> #define PIPEX_STATE_INITIAL 0x0000
> #define PIPEX_STATE_OPENED 0x0001
> #define PIPEX_STATE_CLOSE_WAIT 0x0002
> #define PIPEX_STATE_CLOSE_WAIT2 0x0003
> #define PIPEX_STATE_CLOSED 0x0004
>
> - uint32_t idle_time; /* [N] idle time in seconds */
> + uint32_t idle_time; /* [L] idle time in seconds */
> uint16_t ip_forward:1, /* [N] {en|dis}ableIP forwarding */
> ip6_forward:1, /* [I] {en|dis}able IPv6 forwarding */
> is_multicast:1, /* [I] virtual entry for multicast */
Otherwise OK bluhm@
