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

[Cornelia Huck]

On Fri, Sep 22 2023, Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

> On Thu, 21 Sep 2023 15:14:12 +0800
> Dinghao Liu <dinghao.liu@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
>
> @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
>
> [..]