linux-kernel - Re: [PATCH 01/15] vfio: Add helpers for unifying vfio

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

Date:   Tue, 30 Aug 2022 12:10:08 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Anthony Krowiak <akrowiak@...ux.ibm.com>
Cc:     Kevin Tian <kevin.tian@...el.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>,
        Zhi Wang <zhi.a.wang@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Eric Farman <farman@...ux.ibm.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Vineeth Vijayan <vneethv@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Jason Herne <jjherne@...ux.ibm.com>,
        Harald Freudenberger <freude@...ux.ibm.com>,
        Diana Craciun <diana.craciun@....nxp.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Longfang Liu <liulongfang@...wei.com>,
        Shameer Kolothum <shameerali.kolothum.thodi@...wei.com>,
        Yishai Hadas <yishaih@...dia.com>,
        Eric Auger <eric.auger@...hat.com>,
        Kirti Wankhede <kwankhede@...dia.com>,
        Leon Romanovsky <leon@...nel.org>,
        Abhishek Sahu <abhsahu@...dia.com>,
        intel-gvt-dev@...ts.freedesktop.org,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, Yi Liu <yi.l.liu@...el.com>
Subject: Re: [PATCH 01/15] vfio: Add helpers for unifying vfio_device life
 cycle

On Tue, Aug 30, 2022 at 09:42:42AM -0400, Anthony Krowiak wrote:

> > +/*
> > + * Alloc and initialize vfio_device so it can be registered to vfio
> > + * core.
> > + *
> > + * Drivers should use the wrapper vfio_alloc_device() for allocation.
> > + * @size is the size of the structure to be allocated, including any
> > + * private data used by the driver.
> 
> 
> It seems the purpose of the wrapper is to ensure that the object being
> allocated has as its first field a struct vfio_device object and to return
> its container. Why not just make that a requirement for this function -
> which I would rename vfio_alloc_device - and document it in the prologue?
> The caller can then cast the return pointer or use container_of.

There are three fairly common patterns for this kind of thing

1) The caller open codes everything:

   driver_struct = kzalloc()
   core_init(&driver_struct->core)

2) Some 'get priv' / 'get data' is used instead of container_of():

   core_struct = core_alloc(sizeof(*driver_struct))
   driver_struct = core_get_priv(core_struct)

3) The allocations and initialization are consolidated in the core,
   but we continue to use container_of()

   driver_struct = core_alloc(typeof(*driver_struct))

#1 has a general drawback that people routinely mess up the lifecycle
model and get really confused about when to do kfree() vs put(),
creating bugs.

#2 has a general drawback of not using container_of() at all, and being
a bit confusing in some cases

#3 has the general drawback of being a bit magical, but solves 1 and
2's problems.

I would not fix the struct layout without the BUILD_BUG_ON because
someone will accidently change the order and that becomes a subtle
runtime error - so at a minimum the wrapper macro has to exist to
check that.

If you want to allow a dynamic struct layout and avoid the pitfall of
exposing the user to kalloc/kfree, then you still need the macro, and
it does some more complicated offset stuff.

Having the wrapper macro be entirely type safe is appealing and
reduces code in the drivers, IMHO. Tell it what type you are initing
and get back init'd memory for that type that you always, always free
with a put operation.

Jason