netdev - Re: [PATCH 11/20] octeontx2-af: Add support for stripping STAG/CTAG

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]

Date:   Fri, 9 Nov 2018 12:12:37 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Sunil Kovvuri <sunil.kovvuri@...il.com>
Cc:     Networking <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, linux-soc@...r.kernel.org,
        tduszynski@...vell.com, Sunil Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH 11/20] octeontx2-af: Add support for stripping STAG/CTAG

On Fri, Nov 9, 2018 at 5:29 AM Sunil Kovvuri <sunil.kovvuri@...il.com> wrote:
> On Fri, Nov 9, 2018 at 2:17 AM Arnd Bergmann <arnd@...db.de> wrote:
> > On Thu, Nov 8, 2018 at 7:37 PM <sunil.kovvuri@...il.com> wrote:

> >
> > Here is another instance of bitfields in an interface structure. As
> > before, please try to avoid doing that and use bit shifts and masks
> > instead.
> >
> >        Arnd
>
> No, this struct is not part of communication interface.
> This is used to fill up a register in a bit more readable fashion
> instead of plain bit shifts.

But this is still an interface, isn't it? Writing to the register
implies that there is some hardware that interprets the
bits, so they have to be in the right place.

> ===
> struct nix_rx_vtag_action vtag_action;
>
>         *(u64 *)&vtag_action = 0;
>         vtag_action.vtag0_valid = 1;
>         /* must match type set in NIX_VTAG_CFG */
>         vtag_action.vtag0_type = 0;
>         vtag_action.vtag0_lid = NPC_LID_LA;
>         vtag_action.vtag0_relptr = 12;
>         entry.vtag_action = *(u64 *)&vtag_action;
>
>         /* Set TAG 'action' */
>         rvu_write64(rvu, blkaddr, NPC_AF_MCAMEX_BANKX_TAG_ACT(index, actbank),
>                     entry->vtag_action);

I assume this rvu_write64() does a cpu_to_le64() swap on big-endian,
so the contents again are in the wrong place. I don't see any non-reserved
fields that span an 8-bit boundary, so you can probably rearrange the bits
to make it work, but generally speaking it's better to not rely on how the
compiler lays out bit fields.

        Arnd