On 07/07/20(Tue) 01:01, Vitaliy Makkoveev wrote:
> On Mon, Jul 06, 2020 at 08:47:23PM +0200, Martin Pieuchot wrote:
> > On 06/07/20(Mon) 19:23, Vitaliy Makkoveev wrote:
> > > > On 6 Jul 2020, at 17:36, Martin Pieuchot <[email protected]> wrote:
> > > [...]
> > > Unfortunately you can’t be sure about NET_LOCK() status while you are
> > > in pppac_start(). It was described at this thread [1].
> > >
> > > We have two cases:
> > > 1. pppac_start() called from pppac_output(). NET_LOCK() was inherited.
> >
> > Such recursions should be avoided. if_enqueue() should take care of
> > that.
>
> I suggest to finish the route to if_get(9) before. Updated diff which
> removes pipexintr() below. Just against the most resent source tree.
The tasks are not orthogonal. Making sure the NET_LOCK() is taken
inside the pipex boundaries help for this task as well.
That said the current code is not ready for the proposed diff. At
least `pppx_devs', `pipex_rd_head4' and `pipex_rd_head6' must be
protected/annotated.
What about all the lists/hashtables? They aren't annotated, are they
all protected by the NET_LOCK()?
What about `pppx_ifs' is it only used under the KERNEL_LOCK()?
One comment below:
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.119
> diff -u -p -r1.119 pipex.c
> --- sys/net/pipex.c 6 Jul 2020 20:37:51 -0000 1.119
> +++ sys/net/pipex.c 6 Jul 2020 21:55:17 -0000
> @@ -948,8 +861,26 @@ pipex_ip_output(struct mbuf *m0, struct
> m0->m_flags &= ~(M_BCAST|M_MCAST);
>
> /* output ip packets to the session tunnel */
> - if (pipex_ppp_enqueue(m0, session, &pipexoutq))
> - goto dropped;
> + if (session->is_multicast != 0) {
> + struct pipex_session *session_tmp;
> + struct mbuf *m;
> +
Please add a NET_ASSERT_LOCKED() here to indicate that `pipex_session_list'
needs it.
> + LIST_FOREACH(session_tmp, &pipex_session_list, session_list) {
> + if (session->pipex_iface != session_tmp->pipex_iface)
> + continue;
> + if (session_tmp->ip_forward == 0 &&
> + session_tmp->ip6_forward == 0)
> + continue;
> + m = m_copym(m0, 0, M_COPYALL, M_NOWAIT);
> + if (m == NULL) {
> + session->stat.oerrors++;
> + continue;
> + }
> + pipex_ppp_output(m, session_tmp, PPP_IP);
> + }
> + m_freem(m);
> + } else
> + pipex_ppp_output(m0, session, PPP_IP);
>
> return;
> drop:
