Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

> -Original Message-
> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> Sent: 13 February 2017 14:09
> To: Paul Durrant ; xen-de...@lists.xenproject.org;
> linux-ker...@vger.kernel.org
> Cc: Juergen Gross 
> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> 
> >> How about something like (with rather arbitrary values)
> >>
> >> #define PRIVCMD_DMOP_MAX_NUM_BUFFERS   16
> >> #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ 4096
> >>
> >> and make them part of the interface (i.e. put them into privcmd.h)?
> >
> > Given that the values are arbitrary, I think it may be better to make them
> module params. They can then at least be tweaked if privcmd becomes a
> problem with later dm_ops.
> 
> Making them adjustable is a good thing but we still need default values.
> 

Oh, sure. I think I'll actually go for 16 bufs and a max of 4k per bufs as 
defaults. Patch v3 (including a fix for the arm build) coming shortly.

Cheers,

  Paul

> 
> -boris

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Boris Ostrovsky]

How about something like (with rather arbitrary values)

#define PRIVCMD_DMOP_MAX_NUM_BUFFERS   16
#define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ 4096

and make them part of the interface (i.e. put them into privcmd.h)?


Given that the values are arbitrary, I think it may be better to make them 
module params. They can then at least be tweaked if privcmd becomes a problem 
with later dm_ops.


Making them adjustable is a good thing but we still need default values.


-boris

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

> -Original Message-
> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> Sent: 10 February 2017 17:45
> To: Paul Durrant ; xen-de...@lists.xenproject.org;
> linux-ker...@vger.kernel.org
> Cc: Juergen Gross 
> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> On 02/10/2017 11:28 AM, Paul Durrant wrote:
> >> -Original Message-
> >> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> >> Sent: 10 February 2017 16:18
> >> To: Paul Durrant ; xen-
> de...@lists.xenproject.org;
> >> linux-ker...@vger.kernel.org
> >> Cc: Juergen Gross 
> >> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> >>
> >> On 02/10/2017 09:24 AM, Paul Durrant wrote:
> >>> +static long privcmd_ioctl_dm_op(void __user *udata)
> >>> +{
> >>> + struct privcmd_dm_op kdata;
> >>> + struct privcmd_dm_op_buf *kbufs;
> >>> + unsigned int nr_pages = 0;
> >>> + struct page **pages = NULL;
> >>> + struct xen_dm_op_buf *xbufs = NULL;
> >>> + unsigned int i;
> >>> + long rc;
> >>> +
> >>> + if (copy_from_user(, udata, sizeof(kdata)))
> >>> + return -EFAULT;
> >>> +
> >>> + if (kdata.num == 0)
> >>> + return 0;
> >>> +
> >>> + /*
> >>> +  * Set a tolerable upper limit on the number of buffers
> >>> +  * without being overly restrictive, since we can't easily
> >>> +  * predict what future dm_ops may require.
> >>> +  */
> >> I think this deserves its own macro since it really has nothing to do
> >> with page size, has it? Especially since you are referencing it again
> >> below too.
> >>
> >>
> >>> + if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> >>> + return -E2BIG;
> >>> +
> >>> + kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
> >>> + if (!kbufs)
> >>> + return -ENOMEM;
> >>> +
> >>> + if (copy_from_user(kbufs, kdata.ubufs,
> >>> +sizeof(*kbufs) * kdata.num)) {
> >>> + rc = -EFAULT;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + for (i = 0; i < kdata.num; i++) {
> >>> + if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> >>> +kbufs[i].size)) {
> >>> + rc = -EFAULT;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + nr_pages += DIV_ROUND_UP(
> >>> + offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> >>> + PAGE_SIZE);
> >>> + }
> >>> +
> >>> + /*
> >>> +  * Again, set a tolerable upper limit on the number of pages
> >>> +  * needed to lock all the buffers without being overly
> >>> +  * restrictive, since we can't easily predict the size of
> >>> +  * buffers future dm_ops may use.
> >>> +  */
> >> OTOH, these two cases describe different types of copying (the first one
> >> is for buffer descriptors and the second is for buffers themselves). And
> >> so should they be limited by the same value?
> >>
> > I think there needs to be some limit and limiting the allocation to a page
> was the best I came up with. Can you think of a better one?
> 
> How about something like (with rather arbitrary values)
> 
> #define PRIVCMD_DMOP_MAX_NUM_BUFFERS   16
> #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ 4096
> 
> and make them part of the interface (i.e. put them into privcmd.h)?

Given that the values are arbitrary, I think it may be better to make them 
module params. They can then at least be tweaked if privcmd becomes a problem 
with later dm_ops.

  Paul

> 
> -boris


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[kbuild test robot]

Hi Paul,

[auto build test ERROR on xen-tip/linux-next]
[also build test ERROR on v4.10-rc7 next-20170210]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Paul-Durrant/xen-privcmd-support-for-dm_op-and-restriction/20170211-001520
base:   https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/xen/privcmd.c: In function 'privcmd_ioctl_dm_op':
>> drivers/xen/privcmd.c:673:7: error: implicit declaration of function 
>> 'HYPERVISOR_dm_op' [-Werror=implicit-function-declaration]
 rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
  ^~~~
   cc1: some warnings being treated as errors

vim +/HYPERVISOR_dm_op +673 drivers/xen/privcmd.c

   667  for (i = 0; i < kdata.num; i++) {
   668  set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
   669  xbufs[i].size = kbufs[i].size;
   670  }
   671  
   672  xen_preemptible_hcall_begin();
 > 673  rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
   674  xen_preemptible_hcall_end();
   675  
   676  out:

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Boris Ostrovsky]

On 02/10/2017 11:28 AM, Paul Durrant wrote:
>> -Original Message-
>> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
>> Sent: 10 February 2017 16:18
>> To: Paul Durrant ; xen-de...@lists.xenproject.org;
>> linux-ker...@vger.kernel.org
>> Cc: Juergen Gross 
>> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>
>> On 02/10/2017 09:24 AM, Paul Durrant wrote:
>>> +static long privcmd_ioctl_dm_op(void __user *udata)
>>> +{
>>> +   struct privcmd_dm_op kdata;
>>> +   struct privcmd_dm_op_buf *kbufs;
>>> +   unsigned int nr_pages = 0;
>>> +   struct page **pages = NULL;
>>> +   struct xen_dm_op_buf *xbufs = NULL;
>>> +   unsigned int i;
>>> +   long rc;
>>> +
>>> +   if (copy_from_user(, udata, sizeof(kdata)))
>>> +   return -EFAULT;
>>> +
>>> +   if (kdata.num == 0)
>>> +   return 0;
>>> +
>>> +   /*
>>> +* Set a tolerable upper limit on the number of buffers
>>> +* without being overly restrictive, since we can't easily
>>> +* predict what future dm_ops may require.
>>> +*/
>> I think this deserves its own macro since it really has nothing to do
>> with page size, has it? Especially since you are referencing it again
>> below too.
>>
>>
>>> +   if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
>>> +   return -E2BIG;
>>> +
>>> +   kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
>>> +   if (!kbufs)
>>> +   return -ENOMEM;
>>> +
>>> +   if (copy_from_user(kbufs, kdata.ubufs,
>>> +  sizeof(*kbufs) * kdata.num)) {
>>> +   rc = -EFAULT;
>>> +   goto out;
>>> +   }
>>> +
>>> +   for (i = 0; i < kdata.num; i++) {
>>> +   if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
>>> +  kbufs[i].size)) {
>>> +   rc = -EFAULT;
>>> +   goto out;
>>> +   }
>>> +
>>> +   nr_pages += DIV_ROUND_UP(
>>> +   offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>>> +   PAGE_SIZE);
>>> +   }
>>> +
>>> +   /*
>>> +* Again, set a tolerable upper limit on the number of pages
>>> +* needed to lock all the buffers without being overly
>>> +* restrictive, since we can't easily predict the size of
>>> +* buffers future dm_ops may use.
>>> +*/
>> OTOH, these two cases describe different types of copying (the first one
>> is for buffer descriptors and the second is for buffers themselves). And
>> so should they be limited by the same value?
>>
> I think there needs to be some limit and limiting the allocation to a page 
> was the best I came up with. Can you think of a better one?

How about something like (with rather arbitrary values)

#define PRIVCMD_DMOP_MAX_NUM_BUFFERS   16
#define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ 4096

and make them part of the interface (i.e. put them into privcmd.h)?

-boris


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

> -Original Message-
> From: Boris Ostrovsky [mailto:boris.ostrov...@oracle.com]
> Sent: 10 February 2017 16:18
> To: Paul Durrant ; xen-de...@lists.xenproject.org;
> linux-ker...@vger.kernel.org
> Cc: Juergen Gross 
> Subject: Re: [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> On 02/10/2017 09:24 AM, Paul Durrant wrote:
> > +static long privcmd_ioctl_dm_op(void __user *udata)
> > +{
> > +   struct privcmd_dm_op kdata;
> > +   struct privcmd_dm_op_buf *kbufs;
> > +   unsigned int nr_pages = 0;
> > +   struct page **pages = NULL;
> > +   struct xen_dm_op_buf *xbufs = NULL;
> > +   unsigned int i;
> > +   long rc;
> > +
> > +   if (copy_from_user(, udata, sizeof(kdata)))
> > +   return -EFAULT;
> > +
> > +   if (kdata.num == 0)
> > +   return 0;
> > +
> > +   /*
> > +* Set a tolerable upper limit on the number of buffers
> > +* without being overly restrictive, since we can't easily
> > +* predict what future dm_ops may require.
> > +*/
> 
> I think this deserves its own macro since it really has nothing to do
> with page size, has it? Especially since you are referencing it again
> below too.
> 
> 
> > +   if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> > +   return -E2BIG;
> > +
> > +   kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
> > +   if (!kbufs)
> > +   return -ENOMEM;
> > +
> > +   if (copy_from_user(kbufs, kdata.ubufs,
> > +  sizeof(*kbufs) * kdata.num)) {
> > +   rc = -EFAULT;
> > +   goto out;
> > +   }
> > +
> > +   for (i = 0; i < kdata.num; i++) {
> > +   if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> > +  kbufs[i].size)) {
> > +   rc = -EFAULT;
> > +   goto out;
> > +   }
> > +
> > +   nr_pages += DIV_ROUND_UP(
> > +   offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> > +   PAGE_SIZE);
> > +   }
> > +
> > +   /*
> > +* Again, set a tolerable upper limit on the number of pages
> > +* needed to lock all the buffers without being overly
> > +* restrictive, since we can't easily predict the size of
> > +* buffers future dm_ops may use.
> > +*/
> 
> OTOH, these two cases describe different types of copying (the first one
> is for buffer descriptors and the second is for buffers themselves). And
> so should they be limited by the same value?
> 

I think there needs to be some limit and limiting the allocation to a page was 
the best I came up with. Can you think of a better one?

> > +   if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
> > +   rc = -E2BIG;
> > +   goto out;
> > +   }
> > +
> > +   pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > +   if (!pages) {
> > +   rc = -ENOMEM;
> > +   goto out;
> > +   }
> > +
> > +   xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> > +   if (!xbufs) {
> > +   rc = -ENOMEM;
> > +   goto out;
> > +   }
> > +
> > +   rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
> 
> 
> Aren't those buffers already locked (as Andrew mentioned)? They are
> mmapped with MAP_LOCKED.

No, they're not. The new libxendevicemodel code I have does not make any use of 
xencall or guest handles when privcmd supports the DM_OP ioctl, so the caller 
buffers will not be locked.

> 
> And I also wonder whether we need to take rlimit(RLIMIT_MEMLOCK) into
> account.
> 

Maybe. I'll look at that.

  Paul

> -boris
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Boris Ostrovsky]

On 02/10/2017 09:24 AM, Paul Durrant wrote:
> +static long privcmd_ioctl_dm_op(void __user *udata)
> +{
> + struct privcmd_dm_op kdata;
> + struct privcmd_dm_op_buf *kbufs;
> + unsigned int nr_pages = 0;
> + struct page **pages = NULL;
> + struct xen_dm_op_buf *xbufs = NULL;
> + unsigned int i;
> + long rc;
> +
> + if (copy_from_user(, udata, sizeof(kdata)))
> + return -EFAULT;
> +
> + if (kdata.num == 0)
> + return 0;
> +
> + /*
> +  * Set a tolerable upper limit on the number of buffers
> +  * without being overly restrictive, since we can't easily
> +  * predict what future dm_ops may require.
> +  */

I think this deserves its own macro since it really has nothing to do
with page size, has it? Especially since you are referencing it again
below too.


> + if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> + return -E2BIG;
> +
> + kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
> + if (!kbufs)
> + return -ENOMEM;
> +
> + if (copy_from_user(kbufs, kdata.ubufs,
> +sizeof(*kbufs) * kdata.num)) {
> + rc = -EFAULT;
> + goto out;
> + }
> +
> + for (i = 0; i < kdata.num; i++) {
> + if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> +kbufs[i].size)) {
> + rc = -EFAULT;
> + goto out;
> + }
> +
> + nr_pages += DIV_ROUND_UP(
> + offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> + PAGE_SIZE);
> + }
> +
> + /*
> +  * Again, set a tolerable upper limit on the number of pages
> +  * needed to lock all the buffers without being overly
> +  * restrictive, since we can't easily predict the size of
> +  * buffers future dm_ops may use.
> +  */

OTOH, these two cases describe different types of copying (the first one
is for buffer descriptors and the second is for buffers themselves). And
so should they be limited by the same value?

> + if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
> + rc = -E2BIG;
> + goto out;
> + }
> +
> + pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> + if (!pages) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> + if (!xbufs) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + rc = lock_pages(kbufs, kdata.num, pages, nr_pages);


Aren't those buffers already locked (as Andrew mentioned)? They are
mmapped with MAP_LOCKED.

And I also wonder whether we need to take rlimit(RLIMIT_MEMLOCK) into
account.

-boris




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel