On Fri, Jul 08, 2022 at 05:35:38PM +0900, YASUOKA Masahiko wrote:
> Hello,
>
> On Fri, 8 Jul 2022 00:53:16 +0300
> Vitaliy Makkoveev <m...@openbsd.org> wrote:
> > The `pipex_list_mtx' mutex(9) protects global pipex(4) lists so it need
> > to be taken while we perform this foreach loop.
> >
> > The all sessions loop was reworked to make possible to drop the lock
> > within. This is required because pipex_ppp_output() takes scheduler lock
> > when performs ip_send().
> >
> > Index: sys/net/pipex.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/pipex.c,v
> > retrieving revision 1.143
> > diff -u -p -r1.143 pipex.c
> > --- sys/net/pipex.c 2 Jul 2022 08:50:42 -0000 1.143
> > +++ sys/net/pipex.c 7 Jul 2022 21:52:16 -0000
> > @@ -842,20 +842,39 @@ pipex_ip_output(struct mbuf *m0, struct
> >
> > m0->m_flags &= ~(M_BCAST|M_MCAST);
> >
> > - LIST_FOREACH(session_tmp, &pipex_session_list, session_list) {
> > + mtx_enter(&pipex_list_mtx);
> > +
> > + session_tmp = LIST_FIRST(&pipex_session_list);
> > + while (session_tmp != NULL) {
> > + struct pipex_session *session_save = NULL;
> > +
> > if (session_tmp->ownersc != session->ownersc)
> > - continue;
> > + goto next;
> > if ((session->flags & (PIPEX_SFLAGS_IP_FORWARD |
> > PIPEX_SFLAGS_IP6_FORWARD)) == 0)
> > - continue;
> > + goto next;
> > m = m_copym(m0, 0, M_COPYALL, M_NOWAIT);
> > if (m == NULL) {
> > counters_inc(session->stat_counters,
> > pxc_oerrors);
> > - continue;
> > + goto next;
> > }
> > +
> > + refcnt_take(&session->pxs_refcnt);
>
> this "session" should be session_tmp?
>
Yes, we should check `flags' on `session_tmp'. I intentionally leaved it
as is since I introduced `flags' to replace bit fields. It was not
related to the diffs. I see no reason to fix it because we already
removed PIPEXCSESSION from npppd(8) and we are going to remove it from
kernel.
Also, this logic is useless since we started to unlink session from the
stack before kill it.
> Also, isn't it needed to take reference count on top of the block?
>
We don't need to do this because we keep `pipex_session_list' mutex(9)
held. I take the reference just before release the mutex(9) and make
this session accessible to concurrent delete threads. We need to unlink
session from the `pipex_session_list' before delete it, so we can't take
reference on session with null reference counter.
It makes sense to unlock list just before m_copym(9) call and do mbuf(9)
copy with `pipex_list_mtx' mutex(9) unlocked. With the `flags' check
removal the error path will have only foreign sessions:
session_tmp = LIST_FIRST(&pipex_session_list);
while (session_tmp != NULL) {
struct pipex_session *session_save = NULL;
if (session_tmp->ownersc != session->ownersc)
goto next;
refcnt_take(&session_tmp->pxs_refcnt);
mtx_leave(&pipex_list_mtx);
m = m_copym(m0, 0, M_COPYALL, M_NOWAIT)
if (m != NULL)
pipex_ppp_output(m, session_tmp, PPP_IP);
else
counters_inc(session->stat_counters,
pxc_oerrors);
mtx_enter(&pipex_list_mtx);
session_save = session_tmp;
next:
session_tmp = LIST_NEXT(session_tmp, session_list);
if (session_save != NULL)
pipex_rele_session(session_save);
}
The update diff below. I also found we need to increment 'pxc_oerrors'
counter on `session_tmp' instead of session.
Index: sys/net/pipex.c
===================================================================
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.143
diff -u -p -r1.143 pipex.c
--- sys/net/pipex.c 2 Jul 2022 08:50:42 -0000 1.143
+++ sys/net/pipex.c 8 Jul 2022 14:41:37 -0000
@@ -842,20 +842,38 @@ pipex_ip_output(struct mbuf *m0, struct
m0->m_flags &= ~(M_BCAST|M_MCAST);
- LIST_FOREACH(session_tmp, &pipex_session_list, session_list) {
+ mtx_enter(&pipex_list_mtx);
+
+ session_tmp = LIST_FIRST(&pipex_session_list);
+ while (session_tmp != NULL) {
+ struct pipex_session *session_save = NULL;
+
if (session_tmp->ownersc != session->ownersc)
- continue;
+ goto next;
if ((session->flags & (PIPEX_SFLAGS_IP_FORWARD |
PIPEX_SFLAGS_IP6_FORWARD)) == 0)
- continue;
- m = m_copym(m0, 0, M_COPYALL, M_NOWAIT);
- if (m == NULL) {
- counters_inc(session->stat_counters,
+ goto next;
+
+ refcnt_take(&session_tmp->pxs_refcnt);
+ mtx_leave(&pipex_list_mtx);
+
+ m = m_copym(m0, 0, M_COPYALL, M_NOWAIT)
+ if (m != NULL)
+ pipex_ppp_output(m, session_tmp, PPP_IP);
+ else
+ counters_inc(session_tmp->stat_counters,
pxc_oerrors);
- continue;
- }
- pipex_ppp_output(m, session_tmp, PPP_IP);
+
+ mtx_enter(&pipex_list_mtx);
+ session_save = session_tmp;
+next:
+ session_tmp = LIST_NEXT(session_tmp, session_list);
+ if (session_save != NULL)
+ pipex_rele_session(session_save);
}
+
+ mtx_leave(&pipex_list_mtx);
+
m_freem(m0);
}
