netdev - Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used with bit operation

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+6hz4oSsVbpNiopMe-CzHyJNwaM-1GzPXMaGKAD5OUuH_EMrA@mail.gmail.com>
Date:   Fri, 19 Aug 2016 06:58:58 +0800
From:   Feng Gao <gfree.wind@...il.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     Gao Feng <fgao@...ai8.com>, mostrows@...thlink.net,
        jchapman@...alix.com, "David S. Miller" <davem@...emloft.net>,
        Philp Prindeville <philipp@...fish-solutions.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v1 1/1] pppoe: l2tp: the PPPOX_CONNECTED should be used
 with bit operation

inline.

On Fri, Aug 19, 2016 at 1:44 AM, Guillaume Nault <g.nault@...halink.fr> wrote:
> On Thu, Aug 18, 2016 at 09:59:03AM +0800, fgao@...ai8.com wrote:
>> From: Gao Feng <fgao@...ai8.com>
>>
>> There are some codes in pppoe and l2tp which use the PPPOX_CONNECTED
>> as the value including assignment and condition check.
>> They should keep consistent with other codes.
>>
>> Signed-off-by: Gao Feng <fgao@...ai8.com>
>> ---
>>  v1: Initial Patch
>>
>>  drivers/net/ppp/pppoe.c | 2 +-
>>  net/l2tp/l2tp_ppp.c     | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
>> index 4ddae81..684b773 100644
>> --- a/drivers/net/ppp/pppoe.c
>> +++ b/drivers/net/ppp/pppoe.c
>> @@ -697,7 +697,7 @@ static int pppoe_connect(struct socket *sock, struct sockaddr *uservaddr,
>>                       goto err_put;
>>               }
>>
>> -             sk->sk_state = PPPOX_CONNECTED;
>> +             sk->sk_state |= PPPOX_CONNECTED;
>>
> Using plain assignment makes it clear for the reader that other flags
> are unset. I see no reason for changing this.

I get you. So I don't modify the PPPOX_DEAD assignment.
But I am afraid if there is some case that the flag PPPOX_BOUND is set
before PPPOX_CONNECTED . Then the assignment of "PPPOX_CONNECTED" will
clear the PPPOX_BOUND flag.

>
>> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
>> index d9560aa..3984385 100644
>> --- a/net/l2tp/l2tp_ppp.c
>> +++ b/net/l2tp/l2tp_ppp.c
>> @@ -774,7 +774,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
>>  out_no_ppp:
>>       /* This is how we get the session context from the socket. */
>>       sk->sk_user_data = session;
>> -     sk->sk_state = PPPOX_CONNECTED;
>> +     sk->sk_state |= PPPOX_CONNECTED;
>>
> Same here.
>
>> @@ -856,7 +856,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
>>       error = -ENOTCONN;
>>       if (sk == NULL)
>>               goto end;
>> -     if (sk->sk_state != PPPOX_CONNECTED)
>> +     if (!(sk->sk_state & PPPOX_CONNECTED))
>>
> Looks like it was a bug. This one is worth a separate patch.

Ok, I send another patch for this bug.

Regards
Feng