| Message ID | 1539849103-11726-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: ccw: VFIO CCW cleanup part1 | expand |
On 10/18/2018 09:51 AM, Pierre Morel wrote: > There is a risk that the mediated device is used before all the > data are initialized if it is register too early. How big is the risk? Is this a bug fix (if it is do we need cc stable) or readability improvement? I would prefer a commit message that clearly answers these questions. Otherwise I don't have a strong opinion. Regards, Halil > > Let's register the mediated device when all the data structures > which could be used are initialized. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
On Thu, 18 Oct 2018 15:58:48 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On 10/18/2018 09:51 AM, Pierre Morel wrote: > > There is a risk that the mediated device is used before all the > > data are initialized if it is register too early. > > How big is the risk? Is this a bug fix (if it is do we need cc stable) > or readability improvement? This looks like a bug fix to me, but not sure if it is stable material. I wanted to apply as-is. > > I would prefer a commit message that clearly answers these questions. > > Otherwise I don't have a strong opinion. > > Regards, > Halil > > > > > Let's register the mediated device when all the data structures > > which could be used are initialized. > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >
On 10/20/2018 07:10 PM, Cornelia Huck wrote: > On Thu, 18 Oct 2018 15:58:48 +0200 > Halil Pasic <pasic@linux.ibm.com> wrote: > >> On 10/18/2018 09:51 AM, Pierre Morel wrote: >>> There is a risk that the mediated device is used before all the >>> data are initialized if it is register too early. >> >> How big is the risk? Is this a bug fix (if it is do we need cc stable) >> or readability improvement? > > This looks like a bug fix to me, but not sure if it is stable material. > I wanted to apply as-is. > Isn't atomic_set(&private->avail, 1); sufficient to ensure no mdev is created on top of the parent dev unless private->io_work is already initialized? (->avail an ->state are zero initialized, which for the latter means VFIO_CCW_STATE_NOT_OPER.) Pierre states in the cover letter that this one is a bugfix. But I don't quite get it. If it is an exploitable bug, I think it should be stable material, or? Regardless, I would prefer a better commit message and title. For instance 'if it is register too early' does not seem grammatical to me, and the 'register' is also misleading: I guess, it is not about vfio_ccw_mdev_reg() but the register triggered by mdev create. But no strong feelings here: the patch, if nothing else, contributes to readability, thus it is useful. BTW I do remember not being confident about the synchronization in vfio-ccw, but was hoping all this gets cleared up when the new functions (clear, halt, etc) get implement. Regards, Halil >> >> I would prefer a commit message that clearly answers these questions. >> >> Otherwise I don't have a strong opinion. >> >> Regards, >> Halil >> >>> >>> Let's register the mediated device when all the data structures >>> which could be used are initialized. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> >
On Mon, 22 Oct 2018 19:52:25 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On 10/20/2018 07:10 PM, Cornelia Huck wrote: > > On Thu, 18 Oct 2018 15:58:48 +0200 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > >> On 10/18/2018 09:51 AM, Pierre Morel wrote: > >>> There is a risk that the mediated device is used before all the > >>> data are initialized if it is register too early. > >> > >> How big is the risk? Is this a bug fix (if it is do we need cc stable) > >> or readability improvement? > > > > This looks like a bug fix to me, but not sure if it is stable material. > > I wanted to apply as-is. > > > > Isn't atomic_set(&private->avail, 1); sufficient to ensure no mdev > is created on top of the parent dev unless private->io_work is already > initialized? (->avail an ->state are zero initialized, which for the > latter means VFIO_CCW_STATE_NOT_OPER.) > > Pierre states in the cover letter that this one is a bugfix. But I don't > quite get it. > > If it is an exploitable bug, I think it should be stable material, or? > > Regardless, I would prefer a better commit message and title. For > instance 'if it is register too early' does not seem grammatical to me, > and the 'register' is also misleading: I guess, it is not about > vfio_ccw_mdev_reg() but the register triggered by mdev create. > > But no strong feelings here: the patch, if nothing else, contributes to > readability, thus it is useful. > > BTW I do remember not being confident about the synchronization in vfio-ccw, > but was hoping all this gets cleared up when the new functions (clear, halt, > etc) get implement. OK, I'm not in the position at the moment to do an in-depth discussion of this (conference)... Pierre, can you post an updated description, and Halil, can you let me know if that satisfies your requirements? If I'm going to apply this, I'd like to know whether people are happy with that soon... > > Regards, > Halil > > >> > >> I would prefer a commit message that clearly answers these questions. > >> > >> Otherwise I don't have a strong opinion. > >> > >> Regards, > >> Halil > >> > >>> > >>> Let's register the mediated device when all the data structures > >>> which could be used are initialized. > >>> > >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> > > >
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index f47d16b..33fd53f 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -134,14 +134,14 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) if (ret) goto out_free; - ret = vfio_ccw_mdev_reg(sch); - if (ret) - goto out_disable; - INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo); atomic_set(&private->avail, 1); private->state = VFIO_CCW_STATE_STANDBY; + ret = vfio_ccw_mdev_reg(sch); + if (ret) + goto out_disable; + return 0; out_disable:
There is a risk that the mediated device is used before all the data are initialized if it is register too early. Let's register the mediated device when all the data structures which could be used are initialized. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- drivers/s390/cio/vfio_ccw_drv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)