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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcFse1_gijfhDkyxhBFtd1d-o5_4RO2j2urSXJ_HuZzyg@mail.gmail.com>
Date: Wed, 26 Jul 2023 15:01:44 +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 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)

...

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


-- 
With Best Regards,
Andy Shevchenko