Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220223164420.45344-1-andriy.shevchenko@linux.intel.com> <20220224123620.57fd6c8b@p-imbrenda>
In-Reply-To: <20220224123620.57fd6c8b@p-imbrenda>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 24 Feb 2022 21:51:10 +0200
Message-ID: <CAHp75Vfm-zmzoO0AZTv-3eBjXf0FzHh7tbHRn3DoO7EjukFVig@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        "open list:VFIO DRIVER" <kvm@vger.kernel.org>,
        linux-s390@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Janosch Frank <frankja@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > While in this particular case it would not be a (critical) issue,
> > the pattern itself is bad and error prone in case somebody blindly
> > copies to their code.
> >
> > Don't cast parameter to unsigned long pointer in the bit operations.
> > Instead copy to a local variable on stack of a proper type and use.

...

> > +             struct { /* as a 256-bit bitmap */
> > +                     DECLARE_BITMAP(b, 256);
> > +             } bitmap;
> > +             struct { /* as a set of 64-bit words */
> >                       u64 word[4];
> >               } u64;

> > -     set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> > +     set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>
> wouldn't it be enough to pass gisa->u64.word here?
> then no cast would be necessary

No, it will have the same hidden bugs. As I stated in the commit
message, the pattern is quite bad even if in particular code it would
work.

Thanks, Michael, for pointing out other places. They all need to be fixed.

-- 
With Best Regards,
Andy Shevchenko