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;
From:   Cornelia Huck <cohuck@redhat.com>
To:     Halil Pasic <pasic@linux.ibm.com>,
        Dinghao Liu <dinghao.liu@zju.edu.cn>
Cc:     Vineeth Vijayan <vneethv@linux.ibm.com>,
        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
In-Reply-To: <20230922141700.10895474.pasic@linux.ibm.com>
Organization: "Red Hat GmbH, Sitz: Werner-von-Siemens-Ring 12, D-85630
 Grasbrunn, Handelsregister: Amtsgericht =?utf-8?Q?M=C3=BCnchen=2C?= HRB
 153243,
 =?utf-8?Q?Gesch=C3=A4ftsf=C3=BChrer=3A?= Ryan Barnhart, Charles Cachera,
 Michael O'Neill, Amy
 Ross"
References: <20230921071412.13806-1-dinghao.liu@zju.edu.cn>
 <20230922141700.10895474.pasic@linux.ibm.com>
User-Agent: Notmuch/0.37 (https://notmuchmail.org)
Date:   Fri, 22 Sep 2023 14:25:58 +0200
Message-ID: <87sf76z961.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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
>
> [..]