Re: [PATCH v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[Eric Farman]

On 7/20/26 5:48 PM, Farhan Ali wrote:
> On 7/20/2026 1:19 PM, Eric Farman wrote:
> > The routine cp_free() is called to unpin/free any memory once an I/O
> > is completed successfully, or if cp_prefetch() fails. But if cp_init()
> > fails, and cp->initialized is not enabled, the same routine cannot be
> > used to free all the memory.
> >
> > An attempt to address this exists in ccwchain_handle_ccw(), where a
> > single call to ccwchain_free() is made for the currently-processed
> > CCW segment. But this will leak other segments (created as a result
> > of a Transfer in Channel) that had been allocated as part of the same
> > channel program.
> >
> > Address this by performing the cleanup outside of the recursive
> > ccwchain_handle_ccw()/ccwchain_loop_tic() logic.
> >
> > Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free 
> > in cp_init")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> > ---
> >   drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
> >   1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/ 
> > vfio_ccw_cp.c
> > index 7561aa7d3e01..086d1b54bdb0 100644
> > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > @@ -455,9 +455,6 @@ static int ccwchain_handle_ccw(dma32_t cda, struct 
> > channel_program *cp)
> >       /* Loop for tics on this new chain. */
> >       ret = ccwchain_loop_tic(chain, cp);
> > -    if (ret)
> > -        ccwchain_free(chain);
> > -
> >       return ret;
> >   }
> > @@ -486,6 +483,23 @@ static int ccwchain_loop_tic(struct ccwchain 
> > *chain, struct channel_program *cp)
> >       return 0;
> >   }
> > +static int ccwchain_build_ccws(dma32_t cda, struct channel_program *cp)
> > +{
> > +    struct ccwchain *chain, *temp;
> > +    int ret;
> > +
> > +    ret = ccwchain_handle_ccw(cda, cp);
> > +
> > +    if (ret) {
> > +        /* Cleanup if an error occurred */
> > +        list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, 
> > next) {
> > +            ccwchain_free(chain);
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> >   static int ccwchain_fetch_tic(struct ccw1 *ccw,
> >                     struct channel_program *cp)
> >   {
> > @@ -735,7 +749,7 @@ int cp_init(struct channel_program *cp, union orb 
> > *orb)
> >       memcpy(&cp->orb, orb, sizeof(*orb));
> >       /* Build a ccwchain for the first CCW segment */
> > -    ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> > +    ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
> >       if (!ret)
> >           cp->initialized = true;
>
> The fix looks correct to me and I don't think Sashiko found any issues 
> with this patch (issues identified as pre-existing and i think something 
> this series is trying to fix).

Correct, the two pre-existing findings reported against this patch 
should be addressed by patches 2 and 3, respectively, in this series.

> Reviewed-by: Farhan Ali<alifm@xxxxxxxxxxxxx>

Thank you!

> Thanks
>
> Farhan