| Message ID | fbb44bc85f5dfe4fdaebaf9cb74efcfae4743fba.1562616169.git.alifm@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some vfio-ccw fixes | expand |
On Mon, 8 Jul 2019 16:10:35 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > We don't set cp->initialized to true so calling cp_free > will just return and not do anything. > > Also fix a memory leak where we fail to free a ccwchain > on an error. > > Fixes: 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()") > Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) (...) > @@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) > > /* Build a ccwchain for the first CCW segment */ > ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); > - if (ret) > - cp_free(cp); Now that I look again: it's a bit odd that we set the bit in all cases, even if we have an error. We could move that into the !ret branch that sets ->initialized; but it does not really hurt. > > /* It is safe to force: if it was not set but idals used > * ccwchain_calc_length would have returned an error. The rest of the patch looks good to me.
On 07/09/2019 06:06 AM, Cornelia Huck wrote: > On Mon, 8 Jul 2019 16:10:35 -0400 > Farhan Ali <alifm@linux.ibm.com> wrote: > >> We don't set cp->initialized to true so calling cp_free >> will just return and not do anything. >> >> Also fix a memory leak where we fail to free a ccwchain >> on an error. >> >> Fixes: 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()") >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) > > (...) > >> @@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) >> >> /* Build a ccwchain for the first CCW segment */ >> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); >> - if (ret) >> - cp_free(cp); > > Now that I look again: it's a bit odd that we set the bit in all cases, > even if we have an error. We could move that into the !ret branch that > sets ->initialized; but it does not really hurt. By setting the bit, I am assuming you meant cmd.c64? Yes, it doesn't harm anything but for better code readability you have a good point. I will move it into !ret branch in the first patch since I think it would be more appropriate there, no? > >> >> /* It is safe to force: if it was not set but idals used >> * ccwchain_calc_length would have returned an error. > > The rest of the patch looks good to me. > >
On Tue, 9 Jul 2019 10:07:57 -0400 Farhan Ali <alifm@linux.ibm.com> wrote: > On 07/09/2019 06:06 AM, Cornelia Huck wrote: > > On Mon, 8 Jul 2019 16:10:35 -0400 > > Farhan Ali <alifm@linux.ibm.com> wrote: > > > >> We don't set cp->initialized to true so calling cp_free > >> will just return and not do anything. > >> > >> Also fix a memory leak where we fail to free a ccwchain > >> on an error. > >> > >> Fixes: 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()") > >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com> > >> --- > >> drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > > > > (...) > > > >> @@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) > >> > >> /* Build a ccwchain for the first CCW segment */ > >> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); > >> - if (ret) > >> - cp_free(cp); > > > > Now that I look again: it's a bit odd that we set the bit in all cases, > > even if we have an error. We could move that into the !ret branch that > > sets ->initialized; but it does not really hurt. > > By setting the bit, I am assuming you meant cmd.c64? > > Yes, it doesn't harm anything but for better code readability you have a > good point. I will move it into !ret branch in the first patch since I > think it would be more appropriate there, no? Yes to all of that :) > > > > >> > >> /* It is safe to force: if it was not set but idals used > >> * ccwchain_calc_length would have returned an error. > > > > The rest of the patch looks good to me. > > > > >
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 7622b72..31a04a5 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -421,7 +421,7 @@ static int ccwchain_loop_tic(struct ccwchain *chain, static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp) { struct ccwchain *chain; - int len; + int len, ret; /* Copy 2K (the most we support today) of possible CCWs */ len = copy_from_iova(cp->mdev, cp->guest_cp, cda, @@ -448,7 +448,12 @@ static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp) memcpy(chain->ch_ccw, cp->guest_cp, len * sizeof(struct ccw1)); /* Loop for tics on this new chain. */ - return ccwchain_loop_tic(chain, cp); + ret = ccwchain_loop_tic(chain, cp); + + if (ret) + ccwchain_free(chain); + + return ret; } /* Loop for TICs. */ @@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb) /* Build a ccwchain for the first CCW segment */ ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); - if (ret) - cp_free(cp); /* It is safe to force: if it was not set but idals used * ccwchain_calc_length would have returned an error.
We don't set cp->initialized to true so calling cp_free will just return and not do anything. Also fix a memory leak where we fail to free a ccwchain on an error. Fixes: 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()") Signed-off-by: Farhan Ali <alifm@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)