On Sun, Jun 26, 2022 at 07:45:57PM +0300, Vitaliy Makkoveev wrote:
> This extra serialization is not required. In packet processing path we
> have shared netlock held, but we do read-only access on per session
> `flags' and `ifindex'. We always modify them from ioctl(2) path with
> exclusive netlock held. The rest of pipex(4) session is immutable or
> uses per-session locks.
I was wondering about pipex_enable variable. It is documented as
protected by netlock.
net/pipex.c:int pipex_enable = 0; /* [N] */
But I did not find NET_LOCK() in pipex_sysctl() or its callers.
Should we grab net lock there or in net_sysctl() in the write path?
Although only an integer is set atomically, it looks strange if
such a value is changed while we are in the middle of packet
processing.
> It's not related to this diff, but the per-session `ifindex' could be
> declared as immutable. We only set int to non null value when we link
> pipex(4) session to the stack, and we never change it while session is
> linked. We only use `ifindex' to set `ph_ifidx' of the mbuf(9) which
> will be enqueued. It's absolutely normal to have the mbufs with invalid
> `ph_ifidx'.
I think you are right here.
OK bluhm@
> Index: sys/net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.280
> diff -u -p -r1.280 if_ethersubr.c
> --- sys/net/if_ethersubr.c 26 Jun 2022 15:50:21 -0000 1.280
> +++ sys/net/if_ethersubr.c 26 Jun 2022 16:27:40 -0000
> @@ -540,7 +540,6 @@ ether_input(struct ifnet *ifp, struct mb
> case ETHERTYPE_PPPOE:
> if (m->m_flags & (M_MCAST | M_BCAST))
> goto dropanyway;
> - KERNEL_LOCK();
> #ifdef PIPEX
> if (pipex_enable) {
> struct pipex_session *session;
> @@ -548,12 +547,12 @@ 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
> + KERNEL_LOCK();
> if (etype == ETHERTYPE_PPPOEDISC)
> pppoe_disc_input(m);
> else
