netdev - RE: [PATCH net-next] net: fix recent csum changes

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] [day] [month] [year] [list]

Message-ID: <2eebed09fb46486298e18d5de72d3afe@AcuMS.aculab.com>
Date:   Sat, 4 Dec 2021 19:18:39 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     David Laight <David.Laight@...LAB.COM>,
        'Eric Dumazet' <edumazet@...gle.com>
CC:     kernel test robot <lkp@...el.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "kbuild-all@...ts.01.org" <kbuild-all@...ts.01.org>,
        netdev <netdev@...r.kernel.org>,
        Vladimir Oltean <vladimir.oltean@....com>,
        "David Lebrun" <dlebrun@...gle.com>
Subject: RE: [PATCH net-next] net: fix recent csum changes

From: David Laight <David.Laight@...LAB.COM>
> Sent: 04 December 2021 19:03
> 
> From: Eric Dumazet
> > Sent: 04 December 2021 18:34
> >
> > On Sat, Dec 4, 2021 at 6:00 AM David Laight <David.Laight@...lab.com> wrote:
> > >
> > > From: Eric Dumazet
> > > > Sent: 04 December 2021 04:41
> > > >
> > > > On Fri, Dec 3, 2021 at 7:34 PM kernel test robot <lkp@...el.com> wrote:
> > > > >
> > > > > I love your patch! Perhaps something to improve:
> > > ...
> > > >
> > > > Yes, keeping sparse happy with these checksum is not easy.
> > > >
> > > > I will add and use this helper, unless someone has a better idea.
> > > >
> > > > diff --git a/include/net/checksum.h b/include/net/checksum.h
> > > > index 5b96d5bd6e54532a7a82511ff5d7d4c6f18982d5..5218041e5c8f93cd369a2a3a46add3e6a5e41af7
> > > > 100644
> > > > --- a/include/net/checksum.h
> > > > +++ b/include/net/checksum.h
> > > > @@ -180,4 +180,8 @@ static inline void remcsum_unadjust(__sum16 *psum,
> > > > __wsum delta)
> > > >         *psum = csum_fold(csum_sub(delta, (__force __wsum)*psum));
> > > >  }
> > > >
> > > > +static inline __wsum wsum_negate(__wsum val)
> > > > +{
> > > > +       return (__force __wsum)-((__force u32)val);
> > > > +}
> > > >  #endif
> > >
> > > I was thinking that the expression also requires some comments.
> > > So maybe put a #define / static inline in checksum.h like:
> > >
> > > /* Subract the checksum of a buffer.
> > >  * The domain is __wsum is [1..~0u] (ie excludes zero)
> > >  * so ~csum_partial() cannot be used.
> > >  * The two's compliment gives the right answer provided the old 'csum'
> > >  * isn't zero - which it shouldn't be. */
> > > #define csum_partial_sub(buf, len, csum) (-csum_partial(buf, len, -(csum))
> > >
> > > and then add the annotations there to keep sparse happy there.
> > >
> > > will sparse accept '1 + ~csum' ? The compiler should use negate for it.
> > > It actually makes it slightly more obvious why the code is right.
> >
> > Sparse is not happy with  1 + ~csum,
> >
> > So we unfortunately would need something ugly like
> >
> > (__force __wsum)(1 + ~(__force u32)csum)
> >
> > Which most readers of this code will not find obvious.
> 
> Sparse really isn't helping here at all.
> Perhaps there should be a way of marking a function so that
> sparse just ignores it.
> 
> I also rather dislike that the compiler sees the (u32)csum cast.
> The sparse annotation should really be either __sparse(u32)csum
> or __sparse(u32, csum) to that compiler type checking still applies.
> 
> Perhaps adding:
> #define WSUM(val) (__force __wsum)(val)
> #define U32(csum)  (__force u32)(csum)
> before the 'static inlines' in checksum.h and #undeffing them at the end.
> Then all the functions could be made a little easier to read.
> 
> 	return WSUM(1 + ~U32(csum_partial(buf, len, WSUM(1 + ~U32(csum)));
> 
> is a bit better than the casts - still not nice.

Thinking again...
#ifdef __sparse
	/* Skip offsetting to save 4 casts. */
	return ~csum_partial(buf, len, ~csum);
#else
	/* Offset by one so that zero is never returned. */
	return 1 + ~csum_partial(bufg, len, 1 + ~csum);
#endif

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)