Hi Guillaume,
On Wed, Dec 19, 2018 at 4:38 PM Guillaume Nault <g.na...@alphalink.fr> wrote:
>
> On Wed, Dec 19, 2018 at 02:08:08AM +0200, Sam Protsenko wrote:
> > Extract "Protocol" field decompression code from transport protocols to
> > PPP generic layer, where it actually belongs. As a consequence, this
> > patch fixes incorrect place of PFC decompression in L2TP driver (when
> > it's not PPPOX_BOUND) and also enables this decompression for other
> > protocols, like PPPoE.
> >
> > Protocol field decompression also happens in PPP Multilink Protocol
> > code and in PPP compression protocols implementations (bsd, deflate,
> > mppe). It looks like there is no easy way to get rid of that, so it was
> > decided to leave it as is, but provide those cases with appropriate
> > comments instead.
> >
> Yes, ideally we'd make PPP_PROTO() handle compressed protocol so that
> we wouldn't need to decompress protocol field in place. But other parts
> of the ppp code assume uncompressed protocol field in skb->data, so
> let's stick with the current behaviour for now.
>
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 500bc0027c1b..10c4c8eec995 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -1965,6 +1965,14 @@ ppp_do_recv(struct ppp *ppp, struct sk_buff *skb,
> > struct channel *pch)
> > ppp_recv_unlock(ppp);
> > }
> >
> > +/* Decompress protocol field in PPP header if it's compressed */
> > +static inline void
> > +ppp_decompress_proto(struct sk_buff *skb)
> >
> No need for inline keyword in .c files. Also, no need to split line
> before function name. I know older function declarations of
> ppp_generic.c do this, but let's stick to the general networking stack
> style instead.
>
> > +{
> > + if (skb->data[0] & 0x01)
> > + *(u8 *)skb_push(skb, 1) = 0x00;
> > +}
> > +
> This assumes that we have at least 1 byte of head room and 1 byte of
> linear data. Although that's what the original code did, I find that's
> a bit dangerous. Maybe prefix the function name with '__' and add a
> comment in the function description to warn the reader.
>
> > void
> > ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> > {
> > @@ -1986,6 +1994,7 @@ ppp_input(struct ppp_channel *chan, struct sk_buff
> > *skb)
> > goto done;
> > }
> >
> > + ppp_decompress_proto(skb);
> >
> The pskb_may_pull(skb, 2) at the beginning of the functions is there
> because we're going to read the protocol field. It doesn't make sense to
> decompress it after this test (although technically that mostly works).
>
> However, if we move ppp_decompress_proto() before pskb_may_pull(), then
> callers must respect the requirements of ppp_decompress_proto().
>
> > proto = PPP_PROTO(skb);
> > if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
> > /* put it on the channel queue */
> > @@ -2074,6 +2083,9 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct
> > sk_buff *skb)
> > if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
> > goto err;
> >
> > + /* At this point the "Protocol" field MUST be decompressed, either in
> > + * ppp_decompress_frame() or in ppp_receive_mp_frame().
> > + */
> >
> Or ppp_input(). In the general case (no multilink header and no compression)
> it's ppp_input() that'd decompress the protocol field.
>
> > @@ -2290,9 +2305,11 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff
> > *skb, struct channel *pch)
> >
> > /*
> > * Do protocol ID decompression on the first fragment of each packet.
> > + * We can't wait for this to happen in ppp_input(), because
> > + * ppp_receive_nonmp_frame() expects decompressed protocol field.
> > */
> ppp_input() is not going to be called anyway. I think you can omit the
> middle line entirely.
Thanks for the detailed review! All your points are well taken.
Already sent fixed v2, please review.
