linux-kernel - Re: [PATCH] staging: pi433: using div64_u64() instead of do

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]

Date:   Thu, 10 Feb 2022 13:42:48 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        'Jiapeng Chong' <jiapeng.chong@...ux.alibaba.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Abaci Robot <abaci@...ux.alibaba.com>
Subject: Re: [PATCH] staging: pi433: using div64_u64() instead of do_div()

On Thu, Feb 10, 2022 at 09:21:08AM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 10 February 2022 08:06
> > 
> > On Wed, Feb 09, 2022 at 08:15:13PM +0100, Christophe JAILLET wrote:
> > > Le 21/01/2022 à 14:34, David Laight a écrit :
> > > > From: Jiapeng Chong
> > > > > Sent: 21 January 2022 11:50
> > > > > Subject: [PATCH] staging: pi433: using div64_u64() instead of do_div()
> > > > >
> > > > > Clean the following coccicheck warning:
> > > > >
> > > > > ./drivers/staging/pi433/rf69.c:286:1-7: WARNING: do_div() does a
> > > > > 64-by-32 division, please consider using div64_u64 instead.
> > > >
> > > > That is one of patchcheck's worse warnings.
> > > >
> > > > You need to check the domain of the divisor, not its type.
> > > >
> > > > do_div() exists to avoid expensive 64bit divides when the
> > > > divisor is small.
> > > >
> > > > 	David
> > > >
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)
> > > >
> > > >
> > >
> > > Moreover, the patch is broken by itself.
> > >
> > > See [1] were it was already reported that do_div() and div64_u64() don't
> > > have the same calling convention.
> > >
> > > Looks that div64_u64() and div64_ul() works the same way.
> > 
> > We could mark those as __must_check functions.
> 
> That, and some kind of AI system to filter out untested patches
> from (presumably) students who think that the output from these
> tools 'must be right'.
> 
> Same for all the patches for using swap(), min() LIST_HEAD() etc.
> They are at best churn and make the code harder to read.

Churn is kind of the whole point of staging.  Generally, churn is a net
positive for any subsystem.  It's good to get eyes on the code.

The truth is that I looked at this patch and thought "I don't know
what do_div() does" so I moved on.  This is the first time we've ever
recieved a staging patch to convert do_div().  Next time we will all
know the issues with do_div() better.

Adding a __must_check is a good safety measure as well.

I've looked at adding a Smatch check for ignoring the returns for
functions which have no side effects but I've never completed that
work...  :(

regards,
dan carpenter