Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Sun, 24 Sep 2023 19:58:20 +0200
From:   Halil Pasic <pasic@linux.ibm.com>
To:     Vineeth Vijayan <vneethv@linux.ibm.com>
Cc:     Cornelia Huck <cohuck@redhat.com>,
        Dinghao Liu <dinghao.liu@zju.edu.cn>,
        Peter Oberparleiter <oberpar@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Julian Wiedmann <jwi@linux.ibm.com>,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel
Message-ID: <20230924195820.1a2865c3.pasic@linux.ibm.com>
In-Reply-To: <b9e9eca9-413c-0301-3839-bb7534ac76d2@linux.ibm.com>
References: <20230921071412.13806-1-dinghao.liu@zju.edu.cn>
        <20230922141700.10895474.pasic@linux.ibm.com>
        <87sf76z961.fsf@redhat.com>
        <20230922152035.3ef1cb7e.pasic@linux.ibm.com>
        <b9e9eca9-413c-0301-3839-bb7534ac76d2@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 22 Sep 2023 21:15:48 +0200
Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> On 9/22/23 15:20, Halil Pasic wrote:
> >> Author of 2ec2298412e1 here. If I don't completely misremember things,
> >> this was for the orphanage stuff (i.e. ccw devices that were still kept
> >> as disconnected, like dasd still in use, that had to be moved from their
> >> old subchannel object because a different device appeared on that
> >> subchannel.) That orphanage used a single dummy subchannel for all ccw
> >> devices moved there.
> >>
> >> I have no idea how the current common I/O layer works, but that might
> >> give you a hint about what to look for ????  
> > Yes, that is what the commit states and what the series is about. I hope
> > Vineeth can give us some answers ???? maybe even out of the top of his
> > head... If not, I would trust his judgment on whether figuring things
> > out is worthwhile or not.
> >   
> As Corny mentioned, orphanage is the only case i remember where
> this scenario of dynamically allocated sch->lock  being used. I hope
> you remember the cdev->ccwlock, which is nothing but the copy of
> sch->lock pointer. This is rather a tricky design, where we are using 
> the sch->lock and cdev->ccwlock, which are same pointers.
> Because this sch is exclusively for the cdev ops. But at the same time,
> a CC3 code in the stsch can make the attached device an orphanage and
> remove the sch.
> 
> We have already seen an issue with this approach and had couple of
> discussions about avoiding this pointer usage without using an extra
> lock but do not have a right solution for this now.

Based on your response it seem you do understand the problem but are
struggling to find a solution. You are ahead of me. I'm still at the
stage where I don't understand the problem. I had another look at
that orphanage code, especially at ccw_device_move_to_sch(). Looks
to me that the *(sch->lock) ins not required outlive the *sch and
also that there is no move semantic in place.

Based on that let's take this offline, find a quiet hour and have a look 
at the code and the problem. Maybe I can help with the solution once I
understand the problem -- but maybe not.

Regards,
Halil