| Message ID | 20181109023937.96105-3-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | vfio-ccw channel program rework | expand |
On Fri, 9 Nov 2018 03:39:29 +0100 Eric Farman <farman@linux.ibm.com> wrote: > Direct returns from within a loop are rude, but it doesn't mean it gets > to avoid releasing the memory acquired beforehand. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index ef5ab45d94b3..70a006ba4d05 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) > * orb specified one of the unsupported formats, we defer > * checking for IDAWs in unsupported formats to here. > */ > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { > + kfree(p); > return -EOPNOTSUPP; > + } > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > break; Yes, that looks correct.
On 11/08/2018 09:39 PM, Eric Farman wrote: > Direct returns from within a loop are rude, but it doesn't mean it gets > to avoid releasing the memory acquired beforehand. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index ef5ab45d94b3..70a006ba4d05 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) > * orb specified one of the unsupported formats, we defer > * checking for IDAWs in unsupported formats to here. > */ > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { > + kfree(p); > return -EOPNOTSUPP; > + } > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > break; >
On 09/11/2018 03:39, Eric Farman wrote: > Direct returns from within a loop are rude, but it doesn't mean it gets > to avoid releasing the memory acquired beforehand. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index ef5ab45d94b3..70a006ba4d05 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) > * orb specified one of the unsupported formats, we defer > * checking for IDAWs in unsupported formats to here. > */ > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { > + kfree(p); > return -EOPNOTSUPP; > + } > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > break; > Clearly a bug. Reviewed-by: Pierre Morel<pmorel@linux.ibm.com> I hate this function, I think we really should find a way to avoid these multiple alloc/copy/free of the ccw chain.
On Mon, 12 Nov 2018 11:33:40 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 09/11/2018 03:39, Eric Farman wrote: > > Direct returns from within a loop are rude, but it doesn't mean it gets > > to avoid releasing the memory acquired beforehand. > > > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > --- > > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > > index ef5ab45d94b3..70a006ba4d05 100644 > > --- a/drivers/s390/cio/vfio_ccw_cp.c > > +++ b/drivers/s390/cio/vfio_ccw_cp.c > > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) > > * orb specified one of the unsupported formats, we defer > > * checking for IDAWs in unsupported formats to here. > > */ > > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) > > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { > > + kfree(p); > > return -EOPNOTSUPP; > > + } > > > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > > break; > > > > Clearly a bug. > > Reviewed-by: Pierre Morel<pmorel@linux.ibm.com> > > > I hate this function, I think we really should find a way to avoid these > multiple alloc/copy/free of the ccw chain. > Yeah, that is a bit annoying. Maybe pre-alloc a per-device buffer for it?
On Fri, 9 Nov 2018 03:39:29 +0100 Eric Farman <farman@linux.ibm.com> wrote: > Direct returns from within a loop are rude, but it doesn't mean it gets > to avoid releasing the memory acquired beforehand. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index ef5ab45d94b3..70a006ba4d05 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) > * orb specified one of the unsupported formats, we defer > * checking for IDAWs in unsupported formats to here. > */ > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { > + kfree(p); > return -EOPNOTSUPP; > + } > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > break; Thanks, applied.
On 11/12/2018 05:33 AM, Pierre Morel wrote: > On 09/11/2018 03:39, Eric Farman wrote: >> Direct returns from within a loop are rude, but it doesn't mean it gets >> to avoid releasing the memory acquired beforehand. >> >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_cp.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/s390/cio/vfio_ccw_cp.c >> b/drivers/s390/cio/vfio_ccw_cp.c >> index ef5ab45d94b3..70a006ba4d05 100644 >> --- a/drivers/s390/cio/vfio_ccw_cp.c >> +++ b/drivers/s390/cio/vfio_ccw_cp.c >> @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct >> channel_program *cp) >> * orb specified one of the unsupported formats, we defer >> * checking for IDAWs in unsupported formats to here. >> */ >> - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) >> + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { >> + kfree(p); >> return -EOPNOTSUPP; >> + } >> if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) >> break; >> > > Clearly a bug. > > Reviewed-by: Pierre Morel<pmorel@linux.ibm.com> > > > I hate this function, I think we really should find a way to avoid these > multiple alloc/copy/free of the ccw chain. > +1000 I have an attempt at this locally. But I wanted to take a step back before I send it forward, to make sure what I'm doing isn't worse.
On Fri, 9 Nov 2018 03:39:29 +0100 Eric Farman <farman@linux.ibm.com> wrote: > Direct returns from within a loop are rude, but it doesn't mean it > gets to avoid releasing the memory acquired beforehand. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> Acked-by: Halil Pasic <pasic@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c > b/drivers/s390/cio/vfio_ccw_cp.c index ef5ab45d94b3..70a006ba4d05 > 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct > channel_program *cp) > * orb specified one of the unsupported formats, we > defer > * checking for IDAWs in unsupported formats to here. > */ > - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && > ccw_is_idal(ccw)) > + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && > ccw_is_idal(ccw)) { > + kfree(p); > return -EOPNOTSUPP; > + } > > if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > break;
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index ef5ab45d94b3..70a006ba4d05 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -387,8 +387,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) * orb specified one of the unsupported formats, we defer * checking for IDAWs in unsupported formats to here. */ - if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) + if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw)) { + kfree(p); return -EOPNOTSUPP; + } if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) break;
Direct returns from within a loop are rude, but it doesn't mean it gets to avoid releasing the memory acquired beforehand. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_cp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)