| Message ID | 20220203131418.1319-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] xen: add option to disable GNTTABOP_transfer | expand |
Hi Juergen, On 03/02/2022 13:14, Juergen Gross wrote: > Add a comment to include/public/grant_table.h that GNTTABOP_transfer > is deprecated, in order to discourage new use cases. From the commit message, it is unclear to me why we are discouraging new use cases and indirectly encouraging current users to move away from the feature. Patch #1 seems to imply this is because the feature is not present in Linux upstream. But I don't think this is a sufficient reason to deprecate a feature. A more compelling reason would be that the feature is broken and too complex to fix it. So can you provide more details? As a side note, should we also update SUPPORT.md? Cheers,
On 15.02.22 22:13, Julien Grall wrote: > Hi Juergen, > > On 03/02/2022 13:14, Juergen Gross wrote: >> Add a comment to include/public/grant_table.h that GNTTABOP_transfer >> is deprecated, in order to discourage new use cases. > > From the commit message, it is unclear to me why we are discouraging > new use cases and indirectly encouraging current users to move away from > the feature. > > Patch #1 seems to imply this is because the feature is not present in > Linux upstream. But I don't think this is a sufficient reason to > deprecate a feature. > > A more compelling reason would be that the feature is broken and too > complex to fix it. > > So can you provide more details? It is a feature available for PV domains only, and it is very complex and hasn't been tested for ages. > As a side note, should we also update SUPPORT.md? Good question. Juergen
On 16.02.2022 08:20, Juergen Gross wrote: > On 15.02.22 22:13, Julien Grall wrote: >> As a side note, should we also update SUPPORT.md? > > Good question. I'm not sure here either - talking about individual hypercall sub-ops seems overly small granularity to me for this kind of doc. Plus I don't view deprecation and de-supporting as the same thing. The latter would mean to render unsupported any old XenoLinux which may still be in use, all of the sudden. What would want gaining an entry in any event imo is CHANGELOG.md. Jan
Hi, On 16/02/2022 07:20, Juergen Gross wrote: > On 15.02.22 22:13, Julien Grall wrote: >> Hi Juergen, >> >> On 03/02/2022 13:14, Juergen Gross wrote: >>> Add a comment to include/public/grant_table.h that GNTTABOP_transfer >>> is deprecated, in order to discourage new use cases. >> >> From the commit message, it is unclear to me why we are discouraging >> new use cases and indirectly encouraging current users to move away >> from the feature. >> >> Patch #1 seems to imply this is because the feature is not present in >> Linux upstream. But I don't think this is a sufficient reason to >> deprecate a feature. >> >> A more compelling reason would be that the feature is broken and too >> complex to fix it. >> >> So can you provide more details? > > It is a feature available for PV domains only, and it is very complex > and hasn't been tested for ages. Right. To reply to Jan's e-mail here, shouldn't we also de-support (either completely or security) in this case? My concern here is you wrote this code has been untested for ages (not clear how long) and complex. So potentially this means there are security issues in it. Cheers,
On 17.02.22 20:55, Julien Grall wrote: > Hi, > > On 16/02/2022 07:20, Juergen Gross wrote: >> On 15.02.22 22:13, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 03/02/2022 13:14, Juergen Gross wrote: >>>> Add a comment to include/public/grant_table.h that GNTTABOP_transfer >>>> is deprecated, in order to discourage new use cases. >>> >>> From the commit message, it is unclear to me why we are discouraging >>> new use cases and indirectly encouraging current users to move away >>> from the feature. >>> >>> Patch #1 seems to imply this is because the feature is not present in >>> Linux upstream. But I don't think this is a sufficient reason to >>> deprecate a feature. >>> >>> A more compelling reason would be that the feature is broken and too >>> complex to fix it. >>> >>> So can you provide more details? >> >> It is a feature available for PV domains only, and it is very complex >> and hasn't been tested for ages. > > Right. To reply to Jan's e-mail here, shouldn't we also de-support > (either completely or security) in this case? > > My concern here is you wrote this code has been untested for ages (not > clear how long) and complex. So potentially this means there are > security issues in it. In case we are doing this, we should set the default to disable the transfer functionality. Juergen
Hi Jan, On 16/02/2022 09:29, Jan Beulich wrote: > On 16.02.2022 08:20, Juergen Gross wrote: >> On 15.02.22 22:13, Julien Grall wrote: >>> As a side note, should we also update SUPPORT.md? >> >> Good question. > > I'm not sure here either - talking about individual hypercall sub-ops > seems overly small granularity to me for this kind of doc. Plus I > don't view deprecation and de-supporting as the same thing. The latter > would mean to render unsupported any old XenoLinux which may still be > in use, all of the sudden. I understand this would result to unsupport some old OSes (not clear how old). However, from what Juergen said this feature is untested. To me it sound like we are not confident that we could security support this feature. So I am not sure to understand why we only want to deprecate it. Cheers,
On 24.02.2022 23:55, Julien Grall wrote: > On 16/02/2022 09:29, Jan Beulich wrote: >> On 16.02.2022 08:20, Juergen Gross wrote: >>> On 15.02.22 22:13, Julien Grall wrote: >>>> As a side note, should we also update SUPPORT.md? >>> >>> Good question. >> >> I'm not sure here either - talking about individual hypercall sub-ops >> seems overly small granularity to me for this kind of doc. Plus I >> don't view deprecation and de-supporting as the same thing. The latter >> would mean to render unsupported any old XenoLinux which may still be >> in use, all of the sudden. > > I understand this would result to unsupport some old OSes (not clear how > old). However, from what Juergen said this feature is untested. > > To me it sound like we are not confident that we could security support > this feature. > > So I am not sure to understand why we only want to deprecate it. Not sure what to say: Rendering unsupported however old guests is not a nice step. Hence my concern (which isn't an outright objection). Jan
Hi, On 25/02/2022 08:12, Jan Beulich wrote: > On 24.02.2022 23:55, Julien Grall wrote: >> On 16/02/2022 09:29, Jan Beulich wrote: >>> On 16.02.2022 08:20, Juergen Gross wrote: >>>> On 15.02.22 22:13, Julien Grall wrote: >>>>> As a side note, should we also update SUPPORT.md? >>>> >>>> Good question. >>> >>> I'm not sure here either - talking about individual hypercall sub-ops >>> seems overly small granularity to me for this kind of doc. Plus I >>> don't view deprecation and de-supporting as the same thing. The latter >>> would mean to render unsupported any old XenoLinux which may still be >>> in use, all of the sudden. >> >> I understand this would result to unsupport some old OSes (not clear how >> old). However, from what Juergen said this feature is untested. >> >> To me it sound like we are not confident that we could security support >> this feature. >> >> So I am not sure to understand why we only want to deprecate it. > > Not sure what to say: Rendering unsupported however old guests is not > a nice step. Hence my concern (which isn't an outright objection). In the past we have removed support for feature we deemed unsafe to use and it would require effort to secure it. This is despite the fact that it may be used in production (e.g. PV devices, qemu trad...). So I think the question here is really, do you think we can sensibly security support GNTTABOP_transfer? Cheers,
On 25.02.2022 11:24, Julien Grall wrote: > On 25/02/2022 08:12, Jan Beulich wrote: >> On 24.02.2022 23:55, Julien Grall wrote: >>> On 16/02/2022 09:29, Jan Beulich wrote: >>>> On 16.02.2022 08:20, Juergen Gross wrote: >>>>> On 15.02.22 22:13, Julien Grall wrote: >>>>>> As a side note, should we also update SUPPORT.md? >>>>> >>>>> Good question. >>>> >>>> I'm not sure here either - talking about individual hypercall sub-ops >>>> seems overly small granularity to me for this kind of doc. Plus I >>>> don't view deprecation and de-supporting as the same thing. The latter >>>> would mean to render unsupported any old XenoLinux which may still be >>>> in use, all of the sudden. >>> >>> I understand this would result to unsupport some old OSes (not clear how >>> old). However, from what Juergen said this feature is untested. >>> >>> To me it sound like we are not confident that we could security support >>> this feature. >>> >>> So I am not sure to understand why we only want to deprecate it. >> >> Not sure what to say: Rendering unsupported however old guests is not >> a nice step. Hence my concern (which isn't an outright objection). > > In the past we have removed support for feature we deemed unsafe to use > and it would require effort to secure it. This is despite the fact that > it may be used in production (e.g. PV devices, qemu trad...). > > So I think the question here is really, do you think we can sensibly > security support GNTTABOP_transfer? I don't think it's a question of "can", but of "are we willing to". It would be "can" only if we learned of some seemingly very hard to solve issue. From a workload perspective it would certainly be nice if we didn't have to think about this anymore. Yet like in certain other cases, besides the particular item here I'm also worried of setting a precedent which may then be used to argue for the removal of support for other operations, just to make our lives easier. Jan
On 25.02.22 11:30, Jan Beulich wrote: > On 25.02.2022 11:24, Julien Grall wrote: >> On 25/02/2022 08:12, Jan Beulich wrote: >>> On 24.02.2022 23:55, Julien Grall wrote: >>>> On 16/02/2022 09:29, Jan Beulich wrote: >>>>> On 16.02.2022 08:20, Juergen Gross wrote: >>>>>> On 15.02.22 22:13, Julien Grall wrote: >>>>>>> As a side note, should we also update SUPPORT.md? >>>>>> >>>>>> Good question. >>>>> >>>>> I'm not sure here either - talking about individual hypercall sub-ops >>>>> seems overly small granularity to me for this kind of doc. Plus I >>>>> don't view deprecation and de-supporting as the same thing. The latter >>>>> would mean to render unsupported any old XenoLinux which may still be >>>>> in use, all of the sudden. >>>> >>>> I understand this would result to unsupport some old OSes (not clear how >>>> old). However, from what Juergen said this feature is untested. >>>> >>>> To me it sound like we are not confident that we could security support >>>> this feature. >>>> >>>> So I am not sure to understand why we only want to deprecate it. >>> >>> Not sure what to say: Rendering unsupported however old guests is not >>> a nice step. Hence my concern (which isn't an outright objection). >> >> In the past we have removed support for feature we deemed unsafe to use >> and it would require effort to secure it. This is despite the fact that >> it may be used in production (e.g. PV devices, qemu trad...). >> >> So I think the question here is really, do you think we can sensibly >> security support GNTTABOP_transfer? > > I don't think it's a question of "can", but of "are we willing to". It > would be "can" only if we learned of some seemingly very hard to solve > issue. From a workload perspective it would certainly be nice if we > didn't have to think about this anymore. Yet like in certain other > cases, besides the particular item here I'm also worried of setting > a precedent which may then be used to argue for the removal of support > for other operations, just to make our lives easier. Just one comment: desupporting GNTTABOP_transfer would hit only systems with a XenoLinux dom0. I think those running on a still supported Xen version are really rare these days. Juergen
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h index 7934d7b718..7fbd1c6d10 100644 --- a/xen/include/public/grant_table.h +++ b/xen/include/public/grant_table.h @@ -417,6 +417,8 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_dump_table_t); * GNTTABOP_transfer: Transfer <frame> to a foreign domain. The foreign domain * has previously registered its interest in the transfer via <domid, ref>. * + * This operation is deprecated! Please don't add new use cases! + * * Note that, even if the transfer fails, the specified page no longer belongs * to the calling domain *unless* the error is GNTST_bad_page. *
Add a comment to include/public/grant_table.h that GNTTABOP_transfer is deprecated, in order to discourage new use cases. Signed-off-by: Juergen Gross <jgross@suse.com> --- v2: - new patch --- xen/include/public/grant_table.h | 2 ++ 1 file changed, 2 insertions(+)