linux-kernel - Re: [PATCH 1/2] xen/granttable: Support sub-page grants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1323171726.23681.65.camel@zakaz.uk.xensource.com>
Date:	Tue, 6 Dec 2011 11:42:06 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	"annie.li@...cle.com" <annie.li@...cle.com>
CC:	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"konrad.wilk@...cle.com" <konrad.wilk@...cle.com>,
	"jeremy@...p.org" <jeremy@...p.org>,
	"kurt.hackel@...cle.com" <kurt.hackel@...cle.com>,
	Paul Durrant <Paul.Durrant@...rix.com>
Subject: Re: [PATCH 1/2] xen/granttable: Support sub-page grants

On Tue, 2011-12-06 at 10:56 +0000, annie.li@...cle.com wrote:
> From: Annie Li <annie.li@...cle.com>
> 
>     -- They can't be used to map the page (so can only be used in a GNTTABOP_copy
>        hypercall).
>     -- It's possible to grant access with a finer granularity than whole pages.
>     -- Xen guarantees that they can be revoked quickly (a normal map grant can
>        only be revoked with the cooperation of the domain which has been granted
>        access).
> 

Almost all my comments will apply to the transitive grants patch too.

> Signed-off-by: Annie Li <annie.li@...cle.com>
> ---
>  drivers/xen/grant-table.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  include/xen/grant_table.h |   13 +++++++++++++
>  2 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index bd325fd..7a0f4d1 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -261,6 +261,47 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  }
>  EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);
>  
> +int gnttab_grant_foreign_access_subpage_v2(domid_t domid, unsigned long frame,
> +					   int flags, unsigned page_off,
> +					   unsigned length)

Please drop the v2 suffixes on the public functions.

Any reason not to route these via the ops table for consistency with all
the other ops? Then your availability check becomes a test for NULL fn
pointer rather than a specific version.

> +{
> +	int ref;
> +
> +	ref = get_free_entries(1);
> +	if (unlikely(ref < 0))
> +		return -ENOSPC;
> +
> +	gnttab_grant_foreign_access_ref_subpage_v2(ref, domid, frame, flags,
> +						   page_off, length);
> +
> +	return ref;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_subpage_v2);
> +
> +void gnttab_grant_foreign_access_ref_subpage_v2(grant_ref_t ref, domid_t domid,
> +						unsigned long frame, int flags,
> +						unsigned page_off,
> +						unsigned length)
> +{
> +	BUG_ON(flags & (GTF_accept_transfer | GTF_reading |
> +			GTF_writing | GTF_transitive));
> +	BUG_ON(grant_table_version == 1);

Returning -Esomething might be less drastic? ENOSYS perhaps?

> +	gnttab_shared.v2[ref].sub_page.frame = frame;
> +	gnttab_shared.v2[ref].sub_page.page_off = page_off;
> +	gnttab_shared.v2[ref].sub_page.length = length;
> +	gnttab_shared.v2[ref].hdr.domid = domid;
> +	wmb();
> +	gnttab_shared.v2[ref].hdr.flags =
> +				GTF_permit_access | GTF_sub_page | flags;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access_ref_subpage_v2);
> +
> +bool gnttab_subpage_trans_grants_available(void)
> +{
> +	return grant_table_version == 2;
> +}

It's not clear this adds anything over and above letting the user query
the grant table version. It's hard to tell since there are no users
presented here though. Perhaps separate subpage and transitive functions
would be cleaner?

> +EXPORT_SYMBOL_GPL(gnttab_subpage_trans_grants_available);
> +
>  static int gnttab_query_foreign_access_v1(grant_ref_t ref)
>  {
>  	return gnttab_shared.v1[ref].flags & (GTF_reading|GTF_writing);
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index fea4954..7e43652 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -62,6 +62,15 @@ int gnttab_resume(void);
>  
>  int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
>  				int readonly);
> +int gnttab_grant_foreign_access_subpage_v2(domid_t domid, unsigned long frame,
> +					   int flags, unsigned page_off,
> +					   unsigned length);

> +
> +/*
> + * Are sub-page or transitive grants available on this version of Xen?  Returns
> + * true if they are, and false if they're not.
> + */
> +bool gnttab_subpage_trans_grants_available(void);
>  
>  /*
>   * End access through the given grant reference, iff the grant entry is no
> @@ -108,6 +117,10 @@ void gnttab_cancel_free_callback(struct gnttab_free_callback *callback);
>  
>  void gnttab_grant_foreign_access_ref(grant_ref_t ref, domid_t domid,
>  				     unsigned long frame, int readonly);
> +void gnttab_grant_foreign_access_ref_subpage_v2(grant_ref_t ref, domid_t domid,
> +						unsigned long frame, int flags,
> +						unsigned page_off,
> +						unsigned length);
>  
>  void gnttab_grant_foreign_transfer_ref(grant_ref_t, domid_t domid,
>  				       unsigned long pfn);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/