Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Halil Pasic]

On Thu, 21 Sep 2023 15:14:12 +0800
Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:

> When dma_set_coherent_mask() fails, sch->lock has not been
> freed, which is allocated in css_sch_create_locks(), leading
> to a memleak.
> 
> Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

@Vineeth: Do you know why is the spinlock "*sch->lock" allocated
dynamically and referenced via a pointer instead of making the
spinlock simply a member of struct subchannel and getting rid
of the extra allocation?

I did some archaeology together with Peter. The
lock used to be a member but then commit 2ec2298412e1 ("[S390]
subchannel lock conversion.") switched to (mostly) allocating
the lock separately. Mostly because of this hunk:

@@ -520,9 +530,15 @@ cio_validate_subchannel (struct subchannel *sch, struct subchannel_id schid)
        /* Nuke all fields. */
        memset(sch, 0, sizeof(struct subchannel));
 
-       spin_lock_init(&sch->lock);
+       sch->schid = schid;
+       if (cio_is_console(schid)) {
+               sch->lock = cio_get_console_lock();
+       } else {
+               err = cio_create_sch_lock(sch);
+               if (err)
+                       goto out;
+       }

I did not spend a huge amount of time looking at this but this
is the only reason I found for sch->lock being made a pointer. There may
be others, I'm just saying that is all I've found.

Since 863fc8492734 ("s390/cio: get rid of static console subchannel")
that reason with the console_lock is no more. And that brings me back to
the question: "Why?"

Regards,
Halil

[..]

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Cornelia Huck]

On Fri, Sep 22 2023, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 21 Sep 2023 15:14:12 +0800
> Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
>
>> When dma_set_coherent_mask() fails, sch->lock has not been
>> freed, which is allocated in css_sch_create_locks(), leading
>> to a memleak.
>> 
>> Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>
> @Vineeth: Do you know why is the spinlock "*sch->lock" allocated
> dynamically and referenced via a pointer instead of making the
> spinlock simply a member of struct subchannel and getting rid
> of the extra allocation?
>
> I did some archaeology together with Peter. The
> lock used to be a member but then commit 2ec2298412e1 ("[S390]
> subchannel lock conversion.") switched to (mostly) allocating
> the lock separately. Mostly because of this hunk:
>
> @@ -520,9 +530,15 @@ cio_validate_subchannel (struct subchannel *sch, struct subchannel_id schid)
>         /* Nuke all fields. */
>         memset(sch, 0, sizeof(struct subchannel));
>  
> -       spin_lock_init(&sch->lock);
> +       sch->schid = schid;
> +       if (cio_is_console(schid)) {
> +               sch->lock = cio_get_console_lock();
> +       } else {
> +               err = cio_create_sch_lock(sch);
> +               if (err)
> +                       goto out;
> +       }
>
> I did not spend a huge amount of time looking at this but this
> is the only reason I found for sch->lock being made a pointer. There may
> be others, I'm just saying that is all I've found.

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 :)

>
> Since 863fc8492734 ("s390/cio: get rid of static console subchannel")
> that reason with the console_lock is no more. And that brings me back to
> the question: "Why?"
>
> Regards,
> Halil
>
> [..]

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Halil Pasic]

On Fri, 22 Sep 2023 14:25:58 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> > -       spin_lock_init(&sch->lock);
> > +       sch->schid = schid;
> > +       if (cio_is_console(schid)) {
> > +               sch->lock = cio_get_console_lock();
> > +       } else {
> > +               err = cio_create_sch_lock(sch);
> > +               if (err)
> > +                       goto out;
> > +       }
> >
> > I did not spend a huge amount of time looking at this but this
> > is the only reason I found for sch->lock being made a pointer. There may
> > be others, I'm just saying that is all I've found.  
> 
> 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.


Regards,
Halil

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Vineeth Vijayan]

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.

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Halil Pasic]

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

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Peter Oberparleiter]

On 21.09.2023 09:14, Dinghao Liu wrote:
> When dma_set_coherent_mask() fails, sch->lock has not been
> freed, which is allocated in css_sch_create_locks(), leading
> to a memleak.
> 
> Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>

Looks good to me.

Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>

-- 
Peter Oberparleiter
Linux on IBM Z Development - IBM Germany R&D

Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

[Vasily Gorbik]

On Thu, Oct 05, 2023 at 05:12:54PM +0200, Peter Oberparleiter wrote:
> On 21.09.2023 09:14, Dinghao Liu wrote:
> > When dma_set_coherent_mask() fails, sch->lock has not been
> > freed, which is allocated in css_sch_create_locks(), leading
> > to a memleak.
> > 
> > Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> 
> Looks good to me.
> 
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>

Applied, thank you.