| Message ID | 20180423110113.59385-2-bjsdjshi@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/23/2018 01:01 PM, Dong Jia Shi wrote: > From: Halil Pasic <pasic@linux.vnet.ibm.com> > > If the translation of a channel program fails, we may end up attempting > to clean up (free, unpin) stuff that never got translated (and allocated, > pinned) in the first place. > > By adjusting the lengths of the chains accordingly (so the element that > failed, and all subsequent elements are excluded) cleanup activities > based on false assumptions can be avoided. > > Let's make sure cp_free works properly after cp_prefetch returns with an > error by setting ch_len of a ccw chain to the number of the translated > CCWs on that chain. > > Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> AFAIR we came to the conclusion that this one is stable material. [https://www.spinics.net/lists/kvm/msg166629.html] > --- > drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index 2c7550797ec2..62d66e195304 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp) > * and stores the result to ccwchain list. @cp must have been > * initialized by a previous call with cp_init(). Otherwise, undefined > * behavior occurs. > + * For each chain composing the channel program: > + * - On entry ch_len holds the count of CCW to be translated. > + * - On exit ch_len is adjusted to the count of successfully translated CCW. > + * This allows cp_free to find in ch_len the count of CCW to free in a chain. > * > * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced > * as helpers to do ccw chain translation inside the kernel. Basically > @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp) > for (idx = 0; idx < len; idx++) { > ret = ccwchain_fetch_one(chain, idx, cp); > if (ret) > - return ret; > + goto out_err; > } > } > > return 0; > +out_err: > + /* Only cleanup the chain elements that were actually translated. */ > + chain->ch_len = idx; > + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) { > + chain->ch_len = 0; > + } > + return ret; > } > > /** >
On Mon, 23 Apr 2018 13:01:09 +0200 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > From: Halil Pasic <pasic@linux.vnet.ibm.com> > > If the translation of a channel program fails, we may end up attempting > to clean up (free, unpin) stuff that never got translated (and allocated, > pinned) in the first place. > > By adjusting the lengths of the chains accordingly (so the element that > failed, and all subsequent elements are excluded) cleanup activities > based on false assumptions can be avoided. > > Let's make sure cp_free works properly after cp_prefetch returns with an > error by setting ch_len of a ccw chain to the number of the translated > CCWs on that chain. Should that be cc:stable? This problem has been there probably since we introduced vfio-ccw, no? > > Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index 2c7550797ec2..62d66e195304 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp) > * and stores the result to ccwchain list. @cp must have been > * initialized by a previous call with cp_init(). Otherwise, undefined > * behavior occurs. > + * For each chain composing the channel program: > + * - On entry ch_len holds the count of CCW to be translated. > + * - On exit ch_len is adjusted to the count of successfully translated CCW. > + * This allows cp_free to find in ch_len the count of CCW to free in a chain. s/CCW/CCWs/ (x3)? Can change on applying. > * > * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced > * as helpers to do ccw chain translation inside the kernel. Basically > @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp) > for (idx = 0; idx < len; idx++) { > ret = ccwchain_fetch_one(chain, idx, cp); > if (ret) > - return ret; > + goto out_err; > } > } > > return 0; > +out_err: > + /* Only cleanup the chain elements that were actually translated. */ > + chain->ch_len = idx; > + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) { > + chain->ch_len = 0; > + } > + return ret; > } > > /** Else, looks good.
On 04/23/2018 01:40 PM, Cornelia Huck wrote: > On Mon, 23 Apr 2018 13:01:09 +0200 > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > >> From: Halil Pasic <pasic@linux.vnet.ibm.com> >> >> If the translation of a channel program fails, we may end up attempting >> to clean up (free, unpin) stuff that never got translated (and allocated, >> pinned) in the first place. >> >> By adjusting the lengths of the chains accordingly (so the element that >> failed, and all subsequent elements are excluded) cleanup activities >> based on false assumptions can be avoided. >> >> Let's make sure cp_free works properly after cp_prefetch returns with an >> error by setting ch_len of a ccw chain to the number of the translated >> CCWs on that chain. > > Should that be cc:stable? This problem has been there probably since we > introduced vfio-ccw, no? > Seems emails crossed in flight. Yes I think it should be cc stable and this was broken since the very beginning. >> >> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com> >> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> >> --- >> drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c >> index 2c7550797ec2..62d66e195304 100644 >> --- a/drivers/s390/cio/vfio_ccw_cp.c >> +++ b/drivers/s390/cio/vfio_ccw_cp.c >> @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp) >> * and stores the result to ccwchain list. @cp must have been >> * initialized by a previous call with cp_init(). Otherwise, undefined >> * behavior occurs. >> + * For each chain composing the channel program: >> + * - On entry ch_len holds the count of CCW to be translated. >> + * - On exit ch_len is adjusted to the count of successfully translated CCW. >> + * This allows cp_free to find in ch_len the count of CCW to free in a chain. > > s/CCW/CCWs/ (x3)? > Nod. > Can change on applying. > >> * >> * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced >> * as helpers to do ccw chain translation inside the kernel. Basically >> @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp) >> for (idx = 0; idx < len; idx++) { >> ret = ccwchain_fetch_one(chain, idx, cp); >> if (ret) >> - return ret; >> + goto out_err; >> } >> } >> >> return 0; >> +out_err: >> + /* Only cleanup the chain elements that were actually translated. */ >> + chain->ch_len = idx; >> + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) { >> + chain->ch_len = 0; >> + } >> + return ret; >> } >> >> /** > > Else, looks good. >
On Mon, 23 Apr 2018 13:01:09 +0200 Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote: > From: Halil Pasic <pasic@linux.vnet.ibm.com> > > If the translation of a channel program fails, we may end up attempting > to clean up (free, unpin) stuff that never got translated (and allocated, > pinned) in the first place. > > By adjusting the lengths of the chains accordingly (so the element that > failed, and all subsequent elements are excluded) cleanup activities > based on false assumptions can be avoided. > > Let's make sure cp_free works properly after cp_prefetch returns with an > error by setting ch_len of a ccw chain to the number of the translated > CCWs on that chain. > > Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) Thanks, applied. I'll probably send a pull req after lunch. [Some procedural notes: I've created a new vfio-ccw-fixes branch based on the s390 fixes branch for easier handling. Things targeted for the next release will go on the vfio-ccw branch on top of the s390 features branch, as before. Does that work for everybody? (And I am the only vfio-ccw maintainer with a kernel.org account, right?)]
On 04/25/2018 04:43 AM, Dong Jia Shi wrote: >> [Some procedural notes: I've created a new vfio-ccw-fixes branch based >> on the s390 fixes branch for easier handling. Things targeted for the >> next release will go on the vfio-ccw branch on top of the s390 features >> branch, as before. Does that work for everybody? > I don't see a reason that why it doesn't work for me. > Works for me. >> (And I am the only vfio-ccw maintainer with a kernel.org account, >> right?)] >> > I don't have such an account, and don't know if I need to apply for one. Neither do I have a kernel.org account -- at least for now. Regards, Halil
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 2c7550797ec2..62d66e195304 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -715,6 +715,10 @@ void cp_free(struct channel_program *cp) * and stores the result to ccwchain list. @cp must have been * initialized by a previous call with cp_init(). Otherwise, undefined * behavior occurs. + * For each chain composing the channel program: + * - On entry ch_len holds the count of CCW to be translated. + * - On exit ch_len is adjusted to the count of successfully translated CCW. + * This allows cp_free to find in ch_len the count of CCW to free in a chain. * * The S/390 CCW Translation APIS (prefixed by 'cp_') are introduced * as helpers to do ccw chain translation inside the kernel. Basically @@ -749,11 +753,18 @@ int cp_prefetch(struct channel_program *cp) for (idx = 0; idx < len; idx++) { ret = ccwchain_fetch_one(chain, idx, cp); if (ret) - return ret; + goto out_err; } } return 0; +out_err: + /* Only cleanup the chain elements that were actually translated. */ + chain->ch_len = idx; + list_for_each_entry_continue(chain, &cp->ccwchain_list, next) { + chain->ch_len = 0; + } + return ret; } /**