On Fri, Jul 15, 2022 at 02:25:41PM +0200, Alexander Bluhm wrote:
> On Fri, Jul 15, 2022 at 02:05:12PM +0300, Vitaliy Makkoveev wrote:
> > I see no reason to move 'not_ours' checks outside pipex_common_input()
> > because this introduce code duplication. Also I see no reason to take
> > session lock after pipex_ppp_input() call, because it will be released
> > by caller just after successful return. At least fdrelease() has the
> > same behavior.
>
> This is better. Can we get symmetric mutex enter and leave by
> splitting pipex_common_input() in two funtions? The first part
> needs the lock and checks whether the packet is ours.
>
> Maybe something like this:
>
> mtx_enter(&session->pxs_mtx);
> ...
> if (pipex_isours()) {
> mtx_leave(&session->pxs_mtx);
> pipex_common_input();
> return (NULL);
> }
> ...
> mtx_leave(&session->pxs_mtx);
>
> We could also commit the current code as it is with the locked
> paramter and improve in tree.
I have tried this, but if we want to keep current behavior the code in
the input path will be like this:
mtx_enter(&session->pxs_mtx);
if (pipex_length_valid()) {
if (!rewind)
session->proto.pptp.rcv_gap += nseq;
mtx_leave(&session->pxs_mtx);
goto drop;
}
if (pipex_isours()) {
if (!rewind)
session->proto.pptp.rcv_gap += nseq;
mtx_leave(&session->pxs_mtx);
pipex_common_input();
return (NULL);
}
mtx_leave(&session->pxs_mtx);
I doubt this code duplication is better.
>
> > @@ -1261,8 +1275,8 @@ pipex_pppoe_input(struct mbuf *m0, struc
> > sizeof(struct pipex_pppoe_header), &pppoe);
> >
> > hlen = sizeof(struct ether_header) + sizeof(struct pipex_pppoe_header);
> > - if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length)))
> > - == NULL)
> > + m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length), 0);
> > + if (m0 != NULL)
> > return (NULL);
> > m_freem(m0);
>
> Is this the wrong way around? Should be if (m0 == NULL) return (NULL);
>
Sure. Thanks for catch.
