| Message ID | 1465399864-9711-1-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Wei Liu writes ("[PATCH for-4.7] Revert "libxl: No emulated disk driver for xvdX disk""):
> This reverts c0c099d157cc5bc942afef766cf141628a6380a1.
>
> That commit causes regression on the semantics of our diskspec.
> See [0] for more information.
>
> [0] http://lists.xen.org/archives/html/xen-devel/2016-05/msg02876.html
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Please review carefully about this patch. There are one commit that is
> of interest: ef6cb76026628e26e3d1ae53c50ccde1c3c78b1b
>
> I believe the reverting the snippet in question won't cause security
> issue as described in XSA-142, because the code to create IDE disk still
> checks if the disk is read-only.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
I think this is the right answer for 4.7. In 4.8 we should do
something like one of the proposals being discussed in this thread.
Thanks,
Ian.
On Wed, Jun 08, 2016 at 04:32:13PM +0100, Ian Jackson wrote: > Wei Liu writes ("[PATCH for-4.7] Revert "libxl: No emulated disk driver for xvdX disk""): > > This reverts c0c099d157cc5bc942afef766cf141628a6380a1. > > > > That commit causes regression on the semantics of our diskspec. > > See [0] for more information. > > > > [0] http://lists.xen.org/archives/html/xen-devel/2016-05/msg02876.html > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > Please review carefully about this patch. There are one commit that is > > of interest: ef6cb76026628e26e3d1ae53c50ccde1c3c78b1b > > > > I believe the reverting the snippet in question won't cause security > > issue as described in XSA-142, because the code to create IDE disk still > > checks if the disk is read-only. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > I think this is the right answer for 4.7. In 4.8 we should do > something like one of the proposals being discussed in this thread. > Thanks. And Anthony said on IRC this patch looked good to him. I've queued this for committing to both unstable and 4.7. Wei. > Thanks, > Ian.
On 08/06/16 16:52, Wei Liu wrote: > On Wed, Jun 08, 2016 at 04:32:13PM +0100, Ian Jackson wrote: >> Wei Liu writes ("[PATCH for-4.7] Revert "libxl: No emulated disk driver for xvdX disk""): >>> This reverts c0c099d157cc5bc942afef766cf141628a6380a1. >>> >>> That commit causes regression on the semantics of our diskspec. >>> See [0] for more information. >>> >>> [0] http://lists.xen.org/archives/html/xen-devel/2016-05/msg02876.html >>> >>> Signed-off-by: Wei Liu <wei.liu2@citrix.com> >>> --- >>> Please review carefully about this patch. There are one commit that is >>> of interest: ef6cb76026628e26e3d1ae53c50ccde1c3c78b1b >>> >>> I believe the reverting the snippet in question won't cause security >>> issue as described in XSA-142, because the code to create IDE disk still >>> checks if the disk is read-only. >> >> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> >> >> I think this is the right answer for 4.7. In 4.8 we should do >> something like one of the proposals being discussed in this thread. >> > > Thanks. And Anthony said on IRC this patch looked good to him. > > I've queued this for committing to both unstable and 4.7. CC'ing Olaf and Konrad for their information. :-) -George
On Wed, Jun 08, George Dunlap wrote:
> CC'ing Olaf and Konrad for their information. :-)
I'm fine with the revert.
Olaf
On Wed, Jun 08, 2016 at 06:17:55PM +0200, Olaf Hering wrote: > On Wed, Jun 08, George Dunlap wrote: > > > CC'ing Olaf and Konrad for their information. :-) > > I'm fine with the revert. Me too! Thanks! > > Olaf
On Wed, Jun 08, Konrad Rzeszutek Wilk wrote: > On Wed, Jun 08, 2016 at 06:17:55PM +0200, Olaf Hering wrote: > > On Wed, Jun 08, George Dunlap wrote: > > > > > CC'ing Olaf and Konrad for their information. :-) > > > > I'm fine with the revert. Another case is pvgrub2, which also uses the 'dev' property to name the disk internally "xen/$vdev". Not sure if any generated grub.cfg actually contains tne name. But now that its reverted everything should continue to work as it did in xen-4.6. Olaf
On Thu, Jun 9, 2016 at 2:23 PM, Olaf Hering <olaf@aepfle.de> wrote: > On Wed, Jun 08, Konrad Rzeszutek Wilk wrote: > >> On Wed, Jun 08, 2016 at 06:17:55PM +0200, Olaf Hering wrote: >> > On Wed, Jun 08, George Dunlap wrote: >> > >> > > CC'ing Olaf and Konrad for their information. :-) >> > >> > I'm fine with the revert. > > Another case is pvgrub2, which also uses the 'dev' property to name the > disk internally "xen/$vdev". Not sure if any generated grub.cfg actually > contains tne name. But now that its reverted everything should continue > to work as it did in xen-4.6. Ah, so pvgrub2 follows the xenolinux code, rather than the pvops code? That should definitely be changed ASAP. -George
On Mon, Jun 13, George Dunlap wrote: > Ah, so pvgrub2 follows the xenolinux code, rather than the pvops code? > That should definitely be changed ASAP. What needs to be changed? It has to construct some internal identifier, and appearently "xen/$vdev" was the most obvious choice. Olaf
On 13/06/16 11:21, Olaf Hering wrote: > On Mon, Jun 13, George Dunlap wrote: > >> Ah, so pvgrub2 follows the xenolinux code, rather than the pvops code? >> That should definitely be changed ASAP. > > What needs to be changed? It has to construct some internal identifier, > and appearently "xen/$vdev" was the most obvious choice. I took you to mean that the on-disk grub.cfg would contain references to "hda" rather than "xvda", which would be problematic. If there's no user-visible effect of this choice then of course it doesn't matter in the slightest (and bringing it up was a red herring). -George
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 155a653..6ff05c3 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1422,12 +1422,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, format, &disks[i], colo_mode); - } else if (strncmp(disks[i].vdev, "xvd", 3) == 0) { - /* - * Do not add any emulated disk when PV disk are - * explicitly asked for. - */ - continue; } else if (disk < 6 && b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI) { if (!disks[i].readwrite) { LOG(ERROR, "qemu-xen doesn't support read-only AHCI disk drivers");
This reverts c0c099d157cc5bc942afef766cf141628a6380a1. That commit causes regression on the semantics of our diskspec. See [0] for more information. [0] http://lists.xen.org/archives/html/xen-devel/2016-05/msg02876.html Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Please review carefully about this patch. There are one commit that is of interest: ef6cb76026628e26e3d1ae53c50ccde1c3c78b1b I believe the reverting the snippet in question won't cause security issue as described in XSA-142, because the code to create IDE disk still checks if the disk is read-only. --- tools/libxl/libxl_dm.c | 6 ------ 1 file changed, 6 deletions(-)