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
| ||
|
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
Powered by blists - more mailing lists