netdev - Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock

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-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UdPWPtmatsWtAGTpEMA_SxEqm4OehD3Obg-mA2noBZtOw@mail.gmail.com>
Date:   Mon, 25 Jun 2018 20:25:42 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Amritha Nambiar <amritha.nambiar@...el.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexander Duyck <alexander.h.duyck@...el.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>
Subject: Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in
 sock_common to unsigned short

On Mon, Jun 25, 2018 at 6:34 PM, Tom Herbert <tom@...bertland.com> wrote:
>
>
> On Mon, Jun 25, 2018 at 11:04 AM, Amritha Nambiar
> <amritha.nambiar@...el.com> wrote:
>>
>> Change 'skc_tx_queue_mapping' field in sock_common structure from
>> 'int' to 'unsigned short' type with 0 indicating unset and
>> a positive queue value being set. This way it is consistent with
>> the queue_mapping field in the sk_buff. This will also accommodate
>> adding a new 'unsigned short' field in sock_common in the next
>> patch for rx_queue_mapping.
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
>> ---
>>  include/net/sock.h |   10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index b3b7541..009fd30 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -214,7 +214,7 @@ struct sock_common {
>>                 struct hlist_node       skc_node;
>>                 struct hlist_nulls_node skc_nulls_node;
>>         };
>> -       int                     skc_tx_queue_mapping;
>> +       unsigned short          skc_tx_queue_mapping;
>>         union {
>>                 int             skc_incoming_cpu;
>>                 u32             skc_rcv_wnd;
>> @@ -1681,17 +1681,19 @@ static inline int sk_receive_skb(struct sock *sk,
>> struct sk_buff *skb,
>>
>>  static inline void sk_tx_queue_set(struct sock *sk, int tx_queue)
>>  {
>> -       sk->sk_tx_queue_mapping = tx_queue;
>> +       /* sk_tx_queue_mapping accept only upto a 16-bit value */
>> +       WARN_ON((unsigned short)tx_queue > USHRT_MAX);
>
>
> Shouldn't this be USHRT_MAX - 1 ?

Actually just a ">=" would probably do as well.

>
>> +       sk->sk_tx_queue_mapping = tx_queue + 1;
>>  }
>>
>>  static inline void sk_tx_queue_clear(struct sock *sk)
>>  {
>> -       sk->sk_tx_queue_mapping = -1;
>>
>> +       sk->sk_tx_queue_mapping = 0;
>
>
> I think it's slightly better to define a new constant like NO_QUEUE_MAPPING
> to be USHRT_MAX. That avoids needing to do the arithmetic every time the
> value is accessed.
>>
>>  }
>>
>>  static inline int sk_tx_queue_get(const struct sock *sk)
>>  {
>> -       return sk ? sk->sk_tx_queue_mapping : -1;
>> +       return sk ? sk->sk_tx_queue_mapping - 1 : -1;
>
>
> Doesn't the comparison in __netdev_pick_tx need to be simultaneously changed
> for this?

This doesn't change the result. It was still -1 if the queue mapping
is not set. It was just initialized to 0 instead of to -1 so we have
to perform the operation to get there.

Also in regards to the comment above about needing an extra operation
I am not sure it makes much difference.

In the case of us starting with 0 as a reserved value I think the
instruction count should be about the same. We move the unsigned short
into an unsigned in, then decrement, and if the value is non-negative
we can assume it is valid. Although maybe I should double check the
code to make certain it is doing what I thought it was supposed to be
doing.

>
>>
>>
>>
>>  }
>>
>>  static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>>
>