Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 20 Sep 2021 15:27:44 +0200
From:   Halil Pasic <pasic@linux.ibm.com>
To:     Cornelia Huck <cohuck@redhat.com>
Cc:     Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>,
        Michael Mueller <mimu@linux.ibm.com>,
        linux-s390@vger.kernel.org,
        virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, bfu@redhat.com,
        Vineeth Vijayan <vneethv@linux.ibm.com>,
        Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
Message-ID: <20210920152744.55af1201.pasic@linux.ibm.com>
In-Reply-To: <875yuvh73k.fsf@redhat.com>
References: <20210915215742.1793314-1-pasic@linux.ibm.com>
        <87pmt8hp5o.fsf@redhat.com>
        <20210916151835.4ab512b2.pasic@linux.ibm.com>
        <87mtobh9xn.fsf@redhat.com>
        <20210920003935.1369f9fe.pasic@linux.ibm.com>
        <875yuvh73k.fsf@redhat.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

> On Mon, Sep 20 2021, Halil Pasic <pasic@linux.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