linux-kernel - 回复: [EXT] RE: [PATCH] media: amphion: fix some error related with undefined reference to _

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: <AM6PR04MB634123A8D9F9F60567C979D4E70B9@AM6PR04MB6341.eurprd04.prod.outlook.com>
Date:   Thu, 10 Mar 2022 13:44:50 +0000
From:   Ming Qian <ming.qian@....com>
To:     Robin Murphy <robin.murphy@....com>,
        David Laight <David.Laight@...LAB.COM>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
CC:     "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: 回复: [EXT] RE: [PATCH] media: amphion: fix some error related with undefined reference to __divdi3

> >>
> >> From: Ming Qian
> >>> Sent: 09 March 2022 05:02
> >> ...
> >>> 3. use 'val >> 1' instead of ' val / 2'
> >>
> >> The compiler should do that anyway.
> >>
> >> Especially for unsigned values.
> >> And it has the wrong (different) rounding for negative values.
> >>
> >>          David
> >>
> >
> > Hi David,
> >      Yes, you are right, if the value is negative, the behavior is wrong.
> >      But here, the value type is u32, so I think it's OK.
> 
> Well, it depends on the semantic intent, really. If you're packing a bitfield
> which encodes bits 31:1 of some value then a shift is the most appropriate
> operation. However if you're literally calculating half of a value for, say, a 50%
> threshold level, or the number of 16-bit words represented by a byte length,
> then semantically it's a division, so it should use the divide operator rather
> than obfuscating it behind a shift. Constant division is something that even
> the most basic optimising compiler should handle with ease.
>
Hi Robin,

    Thanks for the detailed explanation, and I agree with you, I will use " / 2" in the v2 patch as it's indeed calculating half of a value.
 

> One more thing that's not the fault of this patch, but stood out in the
> context:
> 
> @@ -1566,7 +1568,7 @@ static bool vpu_malone_check_ready(struct
> vpu_shared_addr *shared, u32 instance)
>         u32 wptr = desc->wptr;
>         u32 used = (wptr + size - rptr) % size;
> 
> -       if (!size || used < size / 2)
> +       if (!size || used < (size >> 1))
>                 return true;
> 
>         return false;
> 
> That's not safe: if "size" is 0 then the undefined behaviour has already
> happened before the "!size" check is reached. If "size" really can be 0, then it
> needs to be checked *before* it is used as a divisor to calculate "used".
> 
> Robin.

Yes, it's problem, and Dan has also pointed it, I 'll fix it in another patch.

Ming