| 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
| ||
|
Message-ID: <431304f4-cbe9-6453-cd3a-0843972ca368@linux.ibm.com>
Date: Wed, 2 Nov 2022 16:00:42 -0400
From: Matthew Rosato <mjrosato@...ux.ibm.com>
To: Eric Farman <farman@...ux.ibm.com>,
Alex Williamson <alex.williamson@...hat.com>,
Cornelia Huck <cohuck@...hat.com>,
Jason Gunthorpe <jgg@...dia.com>,
Kevin Tian <kevin.tian@...el.com>, Yi Liu <yi.l.liu@...el.com>
Cc: 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@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
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>,
Tony Krowiak <akrowiak@...ux.ibm.com>,
Jason Herne <jjherne@...ux.ibm.com>,
Harald Freudenberger <freude@...ux.ibm.com>,
Diana Craciun <diana.craciun@....nxp.com>,
Eric Auger <eric.auger@...hat.com>,
Kirti Wankhede <kwankhede@...dia.com>,
Abhishek Sahu <abhsahu@...dia.com>,
Yishai Hadas <yishaih@...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
Subject: Re: [PATCH v2 1/7] vfio/ccw: create a parent struct
On 11/2/22 3:29 PM, Eric Farman wrote:
> On Wed, 2022-11-02 at 16:01 +0100, Eric Farman wrote:
>> Move the stuff associated with the mdev parent (and thus the
>> subchannel struct) into its own struct, and leave the rest in
>> the existing private structure.
>>
>> The subchannel will point to the parent, and the parent will point
>> to the private, for the areas where one or both are needed. Further
>> separation of these structs will follow.
>>
>> Signed-off-by: Eric Farman <farman@...ux.ibm.com>
>> ---
>> drivers/s390/cio/vfio_ccw_drv.c | 96 ++++++++++++++++++++++++---
>> --
>> drivers/s390/cio/vfio_ccw_ops.c | 8 ++-
>> drivers/s390/cio/vfio_ccw_private.h | 20 ++++--
>> 3 files changed, 100 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
>> b/drivers/s390/cio/vfio_ccw_drv.c
>> index 7f5402fe857a..06022fb37b9d 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
...
>> static int vfio_ccw_sch_probe(struct subchannel *sch)
>> {
>> struct pmcw *pmcw = &sch->schib.pmcw;
>> struct vfio_ccw_private *private;
>> + struct vfio_ccw_parent *parent;
>> int ret = -ENOMEM;
>>
>> if (pmcw->qf) {
>> @@ -213,41 +246,62 @@ static int vfio_ccw_sch_probe(struct subchannel
>> *sch)
>> return -ENODEV;
>> }
>>
>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL);
>> + if (IS_ERR(parent))
>> + return PTR_ERR(parent);
>> +
>> + dev_set_name(&parent->dev, "parent");
>> + parent->dev.parent = &sch->dev;
>> + parent->dev.release = &vfio_ccw_free_parent;
>> + ret = device_register(&parent->dev);
>> + if (ret)
>> + goto out_free;
>> +
>> private = vfio_ccw_alloc_private(sch);
>> - if (IS_ERR(private))
>> + if (IS_ERR(private)) {
>> + put_device(&parent->dev);
>
> This should've been device_unregister. (I could rearrange the code a
> bit to avoid the mix of returns/gotos around here, but since the whole
> series is trying to separate these two structs that seems unnecessary.)
>
>> return PTR_ERR(private);
>> + }
>>
>> - dev_set_drvdata(&sch->dev, private);
>> + dev_set_drvdata(&sch->dev, parent);
>> + dev_set_drvdata(&parent->dev, private);
>>
>> - private->mdev_type.sysfs_name = "io";
>> - private->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
>> - private->mdev_types[0] = &private->mdev_type;
>> - ret = mdev_register_parent(&private->parent, &sch->dev,
>> + parent->mdev_type.sysfs_name = "io";
>> + parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
>> + parent->mdev_types[0] = &parent->mdev_type;
>> + ret = mdev_register_parent(&parent->parent, &sch->dev,
>> &vfio_ccw_mdev_driver,
>> - private->mdev_types, 1);
>> + parent->mdev_types, 1);
>> if (ret)
>> - goto out_free;
>> + goto out_unreg;
>>
>> VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
>> sch->schid.cssid, sch->schid.ssid,
>> sch->schid.sch_no);
>> return 0;
>>
>> +out_unreg:
>> + device_unregister(&parent->dev);
>> out_free:
>> + dev_set_drvdata(&parent->dev, NULL);
>> dev_set_drvdata(&sch->dev, NULL);
>> vfio_ccw_free_private(private);
>> + put_device(&parent->dev);
>
> While this...
>
>> return ret;
>> }
>>
>> static void vfio_ccw_sch_remove(struct subchannel *sch)
>> {
>> - struct vfio_ccw_private *private = dev_get_drvdata(&sch-
>>> dev);
>> + struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
>> + struct vfio_ccw_private *private = dev_get_drvdata(&parent-
>>> dev);
>>
>> - mdev_unregister_parent(&private->parent);
>> + mdev_unregister_parent(&parent->parent);
>>
>> + device_unregister(&parent->dev);
>> dev_set_drvdata(&sch->dev, NULL);
>>
>> vfio_ccw_free_private(private);
>> + put_device(&parent->dev);
>
> ...and this shouldn't even be there. Sorry for the brain fog.
>
Thanks, with these changes I no longer see refcount underflows. I'll continue reviewing with those changes presumed for v3.
Powered by blists - more mailing lists