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

Message-ID: <CAHp75VfP5b4Rv64LZb1e2oCxfjfvNRZvBbGNcOc19tTcUYEjhA@mail.gmail.com>
Date: Wed, 26 Jul 2023 17:32:10 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.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

On Wed, Jul 26, 2023 at 4:17 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
> 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
> >>> On Fri, Jul 21, 2023 at 09:15:28AM +0200, Marcin Szycik wrote:
> >>>> From: Alexander Lobakin <aleksander.lobakin@...el.com>

...

> >>>> +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.

And either you don't need bitmaps (you always operate in the range of
a 32-bit type) or you need to avoid knowing the bitmap internals.
Relying on internal implementation is not a good code, i.e. layering
violation.

> 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

Right.

> With adding bitmap_set_value16() in a separate commit obviously.

Correct.

> That combo shouldn't be too hard for the compiler to optimize into
> a couple writes I hope.

Exactly why I suggested using fixed-width accessors. And if you use
compile-time constants for the bitmaps <= BITS_PER_LONG, it will be
(or at least it should be) optimized to the bitwise ops. That's how
bitmap APIs are done nowadays.

> > 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.

-- 
With Best Regards,
Andy Shevchenko