Re: [PATCH 2/2] gnttab: make grant table v2 support configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.01.2025 14:37, Daniel P. Smith wrote:
> If the v2 interface support is not required, disabling this option will remove
> substantial amounts of unused code in a critical subsystem.
> 
> Disables the v2-only GNTTABOP_get_status_frames grant table op.

Why's this explicitly mentioned, but not e.g. GNTTABOP_{get,set}_version,
which ought to disappear altogether as well is you mean to truly limit
functionality to a pre-v2 hypervisor? Even a post-v2 addition like
GNTTABOP_swap_grant_ref might arguably need disabling then.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1278,8 +1278,8 @@ does not provide `VM_ENTRY_LOAD_GUEST_PAT`.
>  
>  Control various aspects of the grant table behaviour available to guests.
>  
> -* `max-ver` Select the maximum grant table version to offer to guests.  Valid
> -version are 1 and 2.
> +* `max-ver` Select the maximum grant table version to offer to guests. Only
> +available when CONFIG_GRANT_TABLE_V2 is set. Valid version are 1 and 2.

No, the option ought to still be legitimate to use with value 1.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -23,6 +23,24 @@ config GRANT_TABLE
>  
>         If unsure, say Y.
>  
> +config GRANT_TABLE_V2
> +     bool "Grant table version 2 support" if EXPERT

While often I'm a proponent of using EXPERT, here I'm uncertain it
really takes an expert to turn this off. Or wait, it's "to turn
this on", as you have no default at all (see also below). This
means a non-expert has no way to configure a hypervisor compatible
with the previous version; that's a no-go imo.

> +     depends on GRANT_TABLE && X86
> +     help
> +       Grant table interface version 2 is not the default. It has never
> +       been implemented for ARM.
> +
> +       The version 2 interface enables support for systems with large amounts
> +       of memory and some exotic grant primitives that are not in use by the
> +       supported PV drivers.
> +
> +       Disabling this option reduces the amount of complex security-critical
> +       hypervisor code in a subsystem of Xen responsible for approximately
> +       5% of Xen Security Advisories.
> +
> +       If you do not require large memory support, say N.
> +       If you are paranoid, say N. If unsure, say Y.

Should this therefore perhaps have "default BIGMEM"?

> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -296,6 +296,9 @@ int compat_grant_table_op(
>              break;
>  
>          case GNTTABOP_get_status_frames:
> +#ifndef CONFIG_GRANT_TABLE_V2
> +            rc = -ENOSYS;

I understand ENOSYS is abused elsewhere like this, but can we please not
widen the issue and use EOPNOTSUPP instead?

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -59,11 +59,13 @@ struct grant_table {
>      /* Lock protecting the maptrack limit */
>      spinlock_t            maptrack_lock;
>      unsigned int          max_version;
> +#ifdef CONFIG_GRANT_TABLE_V2
>      /*
>       * Defaults to v1.  May be changed with GNTTABOP_set_version.  All other
>       * values are invalid.
>       */
>      unsigned int          gt_version;
> +#endif
>      /* Resource limits of the domain. */
>      unsigned int          max_grant_frames;
>      unsigned int          max_maptrack_frames;

Between this and the following hunk there's also nr_status_frames, which
is v2-only too.

> @@ -178,11 +182,20 @@ static int cf_check 
> parse_gnttab_max_maptrack_frames(const char *arg)
>                                opt_max_maptrack_frames_val);
>  }
>  
> +#ifdef CONFIG_GRANT_TABLE_V2
> +
>  #ifndef GNTTAB_MAX_VERSION
>  #define GNTTAB_MAX_VERSION 2
>  #endif
>  #define get_gt_version(gt) ((gt)->gt_version)
>  
> +#else
> +
> +#define GNTTAB_MAX_VERSION 1
> +#define get_gt_version(gt) 1

What about mem_sharing_gref_to_gfn(), which checks for the version not
having been set yet? gnttab_get_shared_frame_mfn() also has an assertion
to this effect.

> @@ -204,12 +217,17 @@ static int __init cf_check parse_gnttab(const char *s)
>          if ( !strncmp(s, "max-ver:", 8) ||
>               !strncmp(s, "max_ver:", 8) ) /* Alias for original XSA-226 
> patch */
>          {
> +#ifdef CONFIG_GRANT_TABLE_V2
> +            const char *e;
>              long ver = simple_strtol(s + 8, &e, 10);
>  
>              if ( e == ss && ver >= 1 && ver <= 2 )
>                  opt_gnttab_max_version = ver;
>              else
>                  rc = -EINVAL;
> +#else
> +            no_config_param("GRANT_TABLE_V2", "max_ver", s, ss);
> +#endif

See respective comment on the cmdline doc.

> @@ -330,6 +350,14 @@ nr_maptrack_frames(struct grant_table *t)
>  #define status_entry(t, e) \
>      ((t)->status[(e)/STGNT_PER_PAGE][(e)%STGNT_PER_PAGE])
>  
> +#else /* CONFIG_GRANT_TABLE_V2 */
> +
> +#define shared_entry_full_frame(gt, ref) ( shared_entry_v1((gt), 
> (ref)).frame )
> +#define set_shared_entry(gt, ref, val) \
> +    ( shared_entry_v1((gt), (ref)).frame = (val) )
> +#define status_addr(gt, ref, flags_addr) (flags_addr)
> +
> +#endif /* CONFIG_GRANT_TABLE_V2 */

See style related comments on patch 1.

> @@ -734,7 +764,7 @@ static unsigned int nr_grant_entries(struct grant_table 
> *gt)
>          /* Make sure we return a value independently of speculative 
> execution */
>          block_speculation();
>          return f2e(nr_grant_frames(gt), 1);
> -
> +#ifdef CONFIG_GRANT_TABLE_V2
>      case 2:
>          BUILD_BUG_ON(f2e(INITIAL_NR_GRANT_FRAMES, 2) <
>                       GNTTAB_NR_RESERVED_ENTRIES);

Please don't remove blank lines like this.

> @@ -1796,6 +1828,12 @@ static int
>  gnttab_populate_status_frames(struct domain *d, struct grant_table *gt,
>                                unsigned int req_nr_frames)
>  {
> +#ifndef CONFIG_GRANT_TABLE_V2
> +    ASSERT_UNREACHABLE();
> +
> +    return 0;

For a release build you want to return an error here.

> +}

Hmm, the opening figure brace above then has two closing counterparts.
People may find this confusing, and since Misra is almost all about
confusion, I wonder whether they actually permit such (albeit I don't
recall any rule on this matter).

> +#else
>      unsigned int i;
>      unsigned int req_status_frames;
>  
> @@ -1898,6 +1936,7 @@ gnttab_unpopulate_status_frames(struct domain *d, 
> struct grant_table *gt)
>  
>      return 0;
>  }
> +#endif

In fact, to add to the confusion the #else part then even extends across
function boundaries, if the hunk header are to be trusted.

> @@ -2518,12 +2559,14 @@ release_grant_for_copy(
>          td = rd;
>          trans_gref = gref;
>      }
> +#ifdef CONFIG_GRANT_TABLE_V2
>      else
>      {
>          td = (act->src_domid == rd->domain_id)
>               ? rd : knownalive_domain_from_domid(act->src_domid);
>          trans_gref = act->trans_gref;
>      }
> +#endif

Why's this needed? Can't leave it to the compiler's DCE?

> @@ -2748,7 +2792,9 @@ acquire_grant_for_copy(
>              act->is_sub_page = true;
>          }
>      }
> -    else if ( !old_pin ||
> +    else
> +#endif
> +    if ( !old_pin ||
>                (!readonly && !(old_pin & 
> (GNTPIN_devw_mask|GNTPIN_hstw_mask))) )
>      {

Hmm, this #if extending across multiple not really related constructs
looks to be the reason why patch 1 moves the assignment to old_pin.

Below from here is an assignment to act->trans_gref. That's another
field that probably better wouldn't exits in a v1-only build. Much
like is_sub_page and perhaps others.

> @@ -3165,6 +3211,17 @@ static long
>  gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>  {
>      gnttab_set_version_t op;
> +#ifndef CONFIG_GRANT_TABLE_V2
> +
> +    if ( copy_from_guest(&op, uop, 1) )
> +        return -EFAULT;
> +
> +    if ( op.version == 1 )
> +        return 0;
> +
> +    /* Behave as before set_version was introduced. */
> +    return -ENOSYS;

Imo in a case like this one if ( !IS_ENABLED() ) would be preferable
to use, to keep as much code as possible exposed to the compiler,
thus reducing the chance of someone not noticing that it also needs
changing for whatever (perhaps) purely mechanical adjustment. I.e.
use #ifdef / #ifndef only in cases where lexical elements would be
missing, thus breaking the build.

> @@ -4080,6 +4146,9 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  static int gnttab_get_status_frame_mfn(struct domain *d,
>                                         unsigned int idx, mfn_t *mfn)
>  {
> +#ifndef CONFIG_GRANT_TABLE_V2
> +    ASSERT_UNREACHABLE();
> +#else
>      const struct grant_table *gt = d->grant_table;
>  
>      ASSERT(gt->gt_version == 2);
> @@ -4113,6 +4182,7 @@ static int gnttab_get_status_frame_mfn(struct domain *d,
>      /* Make sure idx is bounded wrt nr_status_frames */
>      *mfn = _mfn(virt_to_mfn(
>                  gt->status[array_index_nospec(idx, nr_status_frames(gt))]));
> +#endif
>      return 0;
>  }

As in the earlier case - the function ought to return an error in a
release build.

Jan