netdev - Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing macros instead of literal number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160822095426.bzyexfethxr45rw6@alphalink.fr>
Date:   Mon, 22 Aug 2016 11:54:26 +0200
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Feng Gao <gfree.wind@...il.com>
Cc:     Philp Prindeville <philipp@...fish-solutions.com>,
        Gao Feng <fgao@...vckh6395k16k5.yundunddos.com>,
        paulus@...ba.org, "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 net-next] l2tp: Refactor the codes with existing
 macros instead of literal number

On Mon, Aug 22, 2016 at 08:13:48AM +0800, Feng Gao wrote:
> inline
> 
> On Mon, Aug 22, 2016 at 6:36 AM, Philp Prindeville
> <philipp@...fish-solutions.com> wrote:
> > Inline
> >
> >
> > On 08/20/2016 09:52 AM, fgao@...vckh6395k16k5.yundunddos.com wrote:
> >>
> >> From: Gao Feng <fgao@...ai8.com>
> >>
> >> Use PPP_ALLSTATIONS, PPP_UI, and SEND_SHUTDOWN instead of 0xff,
> >> 0x03, and 2 separately.
> >>
> >> Signed-off-by: Gao Feng <fgao@...ai8.com>
> >> ---
> >>   v3: Modify the subject;
> >>   v2: Only replace the literal number with macros according to Guillaume's
> >> advice
> >>   v1: Inital patch
> >>
> >>   net/l2tp/l2tp_ppp.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> >> index d9560aa..65e2fd6 100644
> >> --- a/net/l2tp/l2tp_ppp.c
> >> +++ b/net/l2tp/l2tp_ppp.c
> >> @@ -177,7 +177,7 @@ static int pppol2tp_recv_payload_hook(struct sk_buff
> >> *skb)
> >>         if (!pskb_may_pull(skb, 2))
> >>                 return 1;
> >>   -     if ((skb->data[0] == 0xff) && (skb->data[1] == 0x03))
> >> +       if ((skb->data[0] == PPP_ALLSTATIONS) && (skb->data[1] == PPP_UI))
> >
> >
> > This should have used PPP_ADDRESS() and PPP_CONTROL() here.
> 
> In my initial patch, I replace them with PPP_ADDRESS() and PPP_CONTROL.
> But Guillaume thought it was not clear as before.
> So I revert it.
> 
> >
> >>                 skb_pull(skb, 2);
> >
> >
> > This magic number should go away.
> 
> Same as above.
> 
> >
> >>         return 0;
> >> @@ -282,7 +282,7 @@ static void pppol2tp_session_sock_put(struct
> >> l2tp_session *session)
> >>   static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
> >>                             size_t total_len)
> >>   {
> >> -       static const unsigned char ppph[2] = { 0xff, 0x03 };
> >> +       static const unsigned char ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
> >
> >
> > PPP has a 4-byte header.  Where's the protocol value?
> 
> In the original code, I fail to find the code which is used to fill
> the protocol value.
> So I keep the two bytes header. And I thought the protocol value may be filled
> by the upper layer.
> 
And you were right. This was a macro replacement patch anyway, so you
didn't have to bring functional changes with it.
And if the protocol field was really missing, the L2TP module would
have never worked.