On 07/03/2019 18.12, Kees Cook wrote: > On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@xxxxxxxxxxxx> wrote: >> >> On Thu, Mar 07, 2019 at 08:52:51AM -0800, Kees Cook wrote: >>> On Thu, Mar 7, 2019 at 7:40 AM Leon Romanovsky <leonro@xxxxxxxxxxxx> wrote: >>>> >>>> On Thu, Mar 07, 2019 at 06:53:54AM -0800, Bart Van Assche wrote: >>>>> On 3/6/19 11:24 PM, Leon Romanovsky wrote: >>>>>> My simple patch passes too :). >>>>> >>>>> Can you repost your patch? >>>> >>>> https://patchwork.kernel.org/patch/10841079/ >>>> >>>> As Rasmus wrote, the thing is to avoid a < 0 check. In my patch, >>>> I converted a <= 0 to !(a > 0 || a == 0) expression. >>> >>> I'd be happy either way. Is there a larger benefit to having a safe >>> "is_non_negative()" helper, or should we go with the minimal change to >>> the shl macro? >> >> I personally prefer simplest possible solution. So, I played around with a few variants on godbolt.org, and it seems that gcc is smart enough to combine (a > 0 || a == 0) into (a >= 0) - in all the cases I tried Leon's patch resulted in the exact same generated code as the current version. Conversely, and rather surprising to me, Bart's patch seemed to cause worse code generation. So now I've changed my mind and also support Leon's version - however, I would _strongly_ prefer if it introduced #define is_non_negative(a) (a > 0 || a == 0) #define is_negative(a) (!(is_non_negative(a)) with appropriate comments and used that. check_shl_overflow is hard enough to read already. Rasmus
