[PATCH] net_sched: force endianness annotation

[PATCH] net_sched: force endianness annotation

[Nicholas Mc Guire]

While the endiannes is being handled correctly sparse was unhappy with
the missing annotation as be16_to_cpu()/be32_to_cpu() expects a __be16
respectively __be32. To mitigate this annotation issue forced annotation
is introduced. Note that this patch has no impact on the generated binary.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem located by an experimental cocci script to locate sparse annontation isues.
net/sched/em_cmp.c:44:31: warning: cast to restricted __be16
net/sched/em_cmp.c:44:31: warning: cast to restricted __be16
net/sched/em_cmp.c:44:31: warning: cast to restricted __be16
net/sched/em_cmp.c:44:31: warning: cast to restricted __be16
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32
net/sched/em_cmp.c:54:31: warning: cast to restricted __be32

This is due to the variable being converted and thus the type is the
teget system type and it must be caset to __be16/__be32 here for sparse
to understand that endianness is adequately handled.

Patch was compile-tested with: x86_64_defconfig

Verification that the patch has no impact on the binary being generated
was done by diff on the binary before and after applying the patch.

Patch is against 5.1-rc6 (localversion-next is next-20190426)

 net/sched/em_cmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
index 1c8360a..3045ee1 100644
--- a/net/sched/em_cmp.c
+++ b/net/sched/em_cmp.c
@@ -41,7 +41,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 		val = get_unaligned_be16(ptr);
 
 		if (cmp_needs_transformation(cmp))
-			val = be16_to_cpu(val);
+			val = be16_to_cpu((__force __be16)val);
 		break;
 
 	case TCF_EM_ALIGN_U32:
@@ -51,7 +51,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
 		val = get_unaligned_be32(ptr);
 
 		if (cmp_needs_transformation(cmp))
-			val = be32_to_cpu(val);
+			val = be32_to_cpu((__force __be32)val);
 		break;
 
 	default:
-- 
2.1.4

Re: [PATCH] net_sched: force endianness annotation

[Cong Wang]

On Sat, Apr 27, 2019 at 11:00 PM Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
> While the endiannes is being handled correctly sparse was unhappy with
> the missing annotation as be16_to_cpu()/be32_to_cpu() expects a __be16
> respectively __be32. To mitigate this annotation issue forced annotation
> is introduced. Note that this patch has no impact on the generated binary.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>


Thanks.

Re: [PATCH] net_sched: force endianness annotation

[Edward Cree]

On 28/04/2019 06:54, Nicholas Mc Guire wrote:
> While the endiannes is being handled correctly sparse was unhappy with
> the missing annotation as be16_to_cpu()/be32_to_cpu() expects a __be16
> respectively __be32.
[...]
> diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
> index 1c8360a..3045ee1 100644
> --- a/net/sched/em_cmp.c
> +++ b/net/sched/em_cmp.c
> @@ -41,7 +41,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
>  		val = get_unaligned_be16(ptr);
>  
>  		if (cmp_needs_transformation(cmp))
> -			val = be16_to_cpu(val);
> +			val = be16_to_cpu((__force __be16)val);
>  		break;
There should probably be a comment here to explain what's going on.  TBH
 it's probably a good general rule that any use of __force should have a
 comment explaining why it's needed.
AFAICT, get_unaligned_be16(ptr) is (barring alignment) equivalent to
 be16_to_cpu(*(__be16 *)ptr).  But then calling be16_to_cpu() again on
 val is bogus; it's already CPU endian.  There's a distinct lack of
 documentation around as to the intended semantics of TCF_EM_CMP_TRANS,
 but it would seem either (__force u16)cpu_to_be16(val); (which preserves
 the existing semantics, that trans is a no-op on BE) or swab16(val);
 would make more sense.

-Ed

Re: [PATCH] net_sched: force endianness annotation

[Nicholas Mc Guire]

On Mon, Apr 29, 2019 at 11:11:20AM +0100, Edward Cree wrote:
> On 28/04/2019 06:54, Nicholas Mc Guire wrote:
> > While the endiannes is being handled correctly sparse was unhappy with
> > the missing annotation as be16_to_cpu()/be32_to_cpu() expects a __be16
> > respectively __be32.
> [...]
> > diff --git a/net/sched/em_cmp.c b/net/sched/em_cmp.c
> > index 1c8360a..3045ee1 100644
> > --- a/net/sched/em_cmp.c
> > +++ b/net/sched/em_cmp.c
> > @@ -41,7 +41,7 @@ static int em_cmp_match(struct sk_buff *skb, struct tcf_ematch *em,
> >  		val = get_unaligned_be16(ptr);
> >  
> >  		if (cmp_needs_transformation(cmp))
> > -			val = be16_to_cpu(val);
> > +			val = be16_to_cpu((__force __be16)val);
> >  		break;
> There should probably be a comment here to explain what's going on.  TBH
>  it's probably a good general rule that any use of __force should have a
>  comment explaining why it's needed.
> AFAICT, get_unaligned_be16(ptr) is (barring alignment) equivalent to
>  be16_to_cpu(*(__be16 *)ptr).  But then calling be16_to_cpu() again on
>  val is bogus; it's already CPU endian.  There's a distinct lack of
>  documentation around as to the intended semantics of TCF_EM_CMP_TRANS,
>  but it would seem either (__force u16)cpu_to_be16(val); (which preserves
>  the existing semantics, that trans is a no-op on BE) or swab16(val);
>  would make more sense.
>
be16_to_cpu((__force __be16)val) should be a NOP on big-endian as well - 
atleast that is how I understood it (usr/include/linux/byteorder/big_endian.h).

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 - while the somewhat
brute force solution is evaluatable simply by diffing.
The swab16() solution seems cleaner than adding another layer of casting - 
but I just am unsure if
-                   val = be16_to_cpu(val);
+                   val = swab16(val);
is actually equivalent. For the original patch this can be checked

-rw-r--r-- 1 hofrat hofrat 2984 Apr 28 01:49 /tmp/em_cmp_force.o
-rw-r--r-- 1 hofrat hofrat 2984 Apr 28 01:49 /tmp/em_cmp_org.o
-rw-r--r-- 1 hofrat hofrat 3392 Apr 29 06:25 /tmp/em_cmp_swab.o
hofrat@debian:~/linux-next$ diff /tmp/em_cmp_force.o /tmp/em_cmp_org.o
hofrat@debian:~/linux-next$

which is why I prefered that solution. if swab16() is equivalent I' resend
a V2

thx!
hofrat

Re: [PATCH] net_sched: force endianness annotation

[Edward Cree]

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.

-Ed

Re: [PATCH] net_sched: force endianness annotation

[Nicholas Mc Guire]

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

Re: [PATCH] net_sched: force endianness annotation

[Christoph Hellwig]

On Sun, Apr 28, 2019 at 07:54:59AM +0200, Nicholas Mc Guire wrote:
> While the endiannes is being handled correctly sparse was unhappy with
> the missing annotation as be16_to_cpu()/be32_to_cpu() expects a __be16
> respectively __be32. To mitigate this annotation issue forced annotation
> is introduced. Note that this patch has no impact on the generated binary.

Every __force needs a comment explaining why it actually іs fine in
this particular case.  Even more bonus points for finding a solution
that does not require the __force.