netdev - Re: [PATCH iwl-next v3 2/6] ip_tunnel: convert _

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: <d5ffe1d3-0378-eaaf-c77f-a1f8a2875826@intel.com>
Date: Wed, 26 Jul 2023 15:16:09 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Andy Shevchenko <andy@...nel.org>, Yury Norov <yury.norov@...il.com>,
	Marcin Szycik <marcin.szycik@...ux.intel.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<wojciech.drewek@...el.com>, <michal.swiatkowski@...ux.intel.com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <jiri@...nulli.us>,
	<pabeni@...hat.com>, <jesse.brandeburg@...el.com>,
	<simon.horman@...igine.com>, <idosch@...dia.com>
Subject: Re: [PATCH iwl-next v3 2/6] ip_tunnel: convert __be16 tunnel flags to
 bitmaps

From: Andy Shevchenko <andy.shevchenko@...il.com>
Date: Wed, 26 Jul 2023 15:01:44 +0300

> On Wed, Jul 26, 2023 at 2:11 PM Alexander Lobakin
> <aleksander.lobakin@...el.com> wrote:
>> From: Andy Shevchenko <andy@...nel.org>, Yury Norov <yury.norov@...il.com>
>> Date: Fri, 21 Jul 2023 17:42:12 +0300
>>
>>> +Cc: Yury on bitmap assignments.
>>
>> I told Marcin to add you to Cc when sending, but forgot Yury, my
>> apologies =\
>>
>>>
>>> (Yury, JFYI,
>>>  if you need the whole series, take message ID as $MSG_ID of this email
>>>  and execute
>>>
>>>    `b4 mbox $MSG_ID`
>>>
>>>  to retrieve it)
>>>
>>> On Fri, Jul 21, 2023 at 09:15:28AM +0200, Marcin Szycik wrote:
>>>> From: Alexander Lobakin <aleksander.lobakin@...el.com>
> 
> ...
> 
>>>> and replace all TUNNEL_* occurencies to
> 
> occurrences
> 
> ...
> 
>>>> otherwise there will be too much conversions
> 
> too many
> (countable)

Ooof :z

> 
> ...
> 
>>>> +static inline void ip_tunnel_flags_from_be16(unsigned long *dst, __be16 flags)
>>>> +{
>>>> +    bitmap_zero(dst, __IP_TUNNEL_FLAG_NUM);
>>>
>>>> +    *dst = be16_to_cpu(flags);
>>>
>>> Oh, This is not good. What you need is something like bitmap_set_value16() in
>>> analogue with bitmap_set_value8().
>>
>> But I don't need `start`, those flag will always be in the first word
>> and I don't need to replace only some range, but to clear everything and
>> then set only the flags which are set in that __be16.
>> Why shouldn't this work?
> 
> I'm not saying it should or shouldn't (actually you need to prove that
> with some test cases added). What I'm saying is that this code is a

Good idea BTW!

> hack because of a layering violation. We do not dereference bitmaps
> with direct access. Even in your code you have bitmap_zero() followed
> by this hack. Why do you call that bitmap_zero() in the first place if
> you are so sure everything will be okay? So either you stick with

Because the bitmap can be longer than one long, but with that direct
deference I only rewrite the first one.

But I admit it's a hack (wasn't hiding that). Just thought this one is
"semi-internal" and it would be okayish to have it... I was wrong :D
What I'm thinking of now is:

	bitmap_zero() // make sure the whole bitmap is cleared
	bitmap_set_value16() // with `start` == 0

With adding bitmap_set_value16() in a separate commit obviously.
That combo shouldn't be too hard for the compiler to optimize into
a couple writes I hope.

> bitops / bitmap APIs or drop all of them and use POD types and bit
> wise ops.
> 
> ...
> 
>>>> +    ret = cpu_to_be16(*flags & U16_MAX);
> 
> Same as above.
> 
> ...
> 
>>>> +    __set_bit(IP_TUNNEL_KEY_BIT, info->key.tun_flags);
>>>> +    __set_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
>>>> +    __set_bit(IP_TUNNEL_NOCACHE_BIT, info->key.tun_flags);
>>>>      if (flags & BPF_F_DONT_FRAGMENT)
>>>> -            info->key.tun_flags |= TUNNEL_DONT_FRAGMENT;
>>>> +            __set_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, info->key.tun_flags);
>>>>      if (flags & BPF_F_ZERO_CSUM_TX)
>>>> -            info->key.tun_flags &= ~TUNNEL_CSUM;
>>>> +            __clear_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
>>>
>>> Instead of set/clear, use assign, i.e. __asign_bit().
>>
>> Just to make it clear, you mean
>>
>>         __assign_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags,
>>                      flags & BPF_F_ZERO_CSUM_TX);
>>
>> right?
> 
> Yes.
> 
> 

Thanks,
Olek