netdev - Re: [PATCH] net_sched: force endianness annotation

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: <20190429112950.GB17830@osadl.at>
Date:   Mon, 29 Apr 2019 13:29:50 +0200
From:   Nicholas Mc Guire <der.herr@...r.at>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Nicholas Mc Guire <hofrat@...dl.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net_sched: force endianness annotation

On Mon, Apr 29, 2019 at 12:11:18PM +0100, Edward Cree wrote:
> On 29/04/2019 11:44, Nicholas Mc Guire wrote:
> > be16_to_cpu((__force __be16)val) should be a NOP on big-endian as well
> Yes.  But it's semiotically wrong to call be16_to_cpu() on a cpu-endian
>  value; if the existing behaviour is desired, it ought to be implemented
>  differently.
> > The problem with using swab16 is that it is impating the binary significantly
> > so I'm not sure if the change is really side-effect free
> It's not; it changes the behaviour.  That's why I brought up the question
>  of the intended behaviour ??? it's unclear whether the current (no-op on BE)
>  behaviour is correct or whether it's a bug in the original code.
> Better to leave the sparse error in place ??? drawing future developers'
>  attention to something being possibly wrong here ??? than to mask it with a
>  synthetic 'fix' which we don't even know if it's correct or not.
> 
> > but I just am unsure if
> > -                   val = be16_to_cpu(val);
> > +                   val = swab16(val);
> > is actually equivalent.
> If you're not sure about such things, maybe you shouldn't be touching
>  endianness-related code.  swab is *not* a no-op, either on BE or LE.

Well the only way to understand it is to try to understand it by reviewing
the implementatoins - which is whyt I'm currently doing - the principle 
issues are clear I think - following the details of the macro-chains is
not always that clear. From looking at the code history it does seem correct
which is why it seemed reasonable to remove the sparse warning and doing
so with a patch that does not change the binary seems the safest.

thx!
hofrat