linux-kernel - Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

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]

Message-ID: <20210920152744.55af1201.pasic@linux.ibm.com>
Date:   Mon, 20 Sep 2021 15:27:44 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>,
        Michael Mueller <mimu@...ux.ibm.com>,
        linux-s390@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, bfu@...hat.com,
        Vineeth Vijayan <vneethv@...ux.ibm.com>,
        Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown

On Mon, 20 Sep 2021 12:30:39 +0200
Cornelia Huck <cohuck@...hat.com> wrote:

> On Mon, Sep 20 2021, Halil Pasic <pasic@...ux.ibm.com> wrote:
[..]
> 
> Basically, the vcdev is supposed to be around while the ccw device is
> online (with a tail end until references have been given up, of course.)
> It embeds a virtio device that has the ccw device as a parent, which
> will give us a reference on the ccw device as long as the virtio device
> is alive. Any interactions with the ccw device (except freeing the dma
> buffer) are limited to the time where we still have a reference to it
> via the virtio device.
>

I didn't remember that device_add() takes a reference to the parent, and
that device_del() before device_put(dev) and remove callback.


> >  
> >>   
> >> > So my intuition tells me that
> >> > drivers should manage explicitly. Yes virtio_ccw happens to have dma
> >> > memory whose lifetime is more or less the lifetime of struct virtio_ccw,
> >> > but that may not be always the case.    
> >> 
> >> I'm not sure what you're getting at here. Regardless of the lifetime of
> >> the dma memory, it depends on the presence of the ccw device to which it
> >> is tied. This means that the ccw device must not be released while the
> >> dma memory is alive. We can use the approach in your patch here due to
> >> the lifetime of the dma memory that virtio-ccw allocates when we start
> >> using the device and frees when we stop using the device, or we can use
> >> get/put with every allocate/release dma memory pair, which should be
> >> safe for everyone?
> >>   
> >
> > What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> > ccw_device. If we get/put in those we can ensure that, provided the
> > alloc and the free calls are properly paired, the device will be still
> > alive (and the pointer valid) for the free, if it was valid for the
> > alloc. But it does not ensure that each and every call to alloc is with
> > a valid pointer, or that other uses of the pointer are OK. So I don't
> > think it is completely safe for everyone, because we could try to use
> > a pointer to a ccw device when not having any dma memory allocated from
> > its pool.  
> 
> But the problem is the dma memory, right? Also, it is the same issue for
> any potential caller of the ccw_device_dma_* interfaces.

I tend to agree, my argument was based on the assumption that we did not
use to take a reference to the ccw device in virtio_ccw_online(), but we
do via register_virtio_device(). This reference however gets dropped
right before virtio_ccw_release_dev() is called.
> 
> >
> > This patch takes reference to cdev before the pointer is published via
> > vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> > that the pointee basically outlives the pointer. (Without having a full
> > understanding of how things are synchronized).  
> 
> I don't think we have to care about accessing ->cdev (see above.) Plus,
> as we give up the dma memory at the very last point, we would also give
> up the reference via that memory at the very last point, so I'm not sure
> what additional problems could come up.

I understand now. Let me think about it some more. I'm wonderning about
leafs. Will come back at you shortly.

Regards,
Halil