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: <CA+6hz4qoHeQ+eD9j21dZS0KQ8_1cnAkj7j0X4msZaGTBDC3ezQ@mail.gmail.com>
Date:   Mon, 22 Aug 2016 18:22:42 +0800
From:   Feng Gao <gfree.wind@...il.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     Gao Feng <fgao@...ai8.com>, paulus@...ba.org,
        "David S. Miller" <davem@...emloft.net>,
        Philp Prindeville <philipp@...fish-solutions.com>,
        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

inline

On Mon, Aug 22, 2016 at 6:07 PM, Guillaume Nault <g.nault@...halink.fr> wrote:
> On Sat, Aug 20, 2016 at 11:52:27PM +0800, fgao@...ai8.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))
>>               skb_pull(skb, 2);
>>
>>       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};
>>
> Minor nit: I'd prefer to keep the space after '{' and before '}'.
> I didn't want to bother you with this, but since it seems you'll have
> to repost...

I don't know if it is the coding style of Linux kernel.

>
>>       struct sock *sk = sock->sk;
>>       struct sk_buff *skb;
>>       int error;
>> @@ -369,7 +369,7 @@ error:
>>   */
>>  static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>>  {
>> -     static const u8 ppph[2] = { 0xff, 0x03 };
>> +     static const u8 ppph[2] = {PPP_ALLSTATIONS, PPP_UI};
>>
> Same here.
>
> BTW, I thought you also wanted to remove the static ppph variable
> from pppol2tp_xmit() / pppol2tp_sendmsg(), to directly assign
> skb->data[0/1] with PPP_ALLSTATIONS/PPP_UI.

If removed static ppph, there will be some codes which use literal "2"
instead of sizeof ppph.
Is it ok?

Regards
Feng