nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
@ 2018-08-13 12:02 Zhang Yi
  2018-08-20 17:53 ` Verma, Vishal L
  0 siblings, 1 reply; 7+ messages in thread
From: Zhang Yi @ 2018-08-13 12:02 UTC (permalink / raw)
  To: linux-kernel, linux-nvdimm, dan.j.williams, jack, zwisler,
	dave.jiang, yu.c.zhang
  Cc: yi.z.zhang

This patch prevents a user mapping an illegal vma range that is larger
than a dax device physical resource.

When qemu maps the dax device for virtual nvdimm's backend device, the
v-nvdimm label area is defined at the end of mapped range. By using an
illegal size that exceeds the range of the device dax, it will trigger a
fault with qemu.

Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
---
 drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 108c37f..6fe8c30 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -177,6 +177,33 @@ static const struct attribute_group *dax_attribute_groups[] = {
 	NULL,
 };
 
+static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma,
+		const char *func)
+{
+	struct device *dev = &dev_dax->dev;
+	struct resource *res;
+	unsigned long size;
+	int ret, i;
+
+	if (!dax_alive(dev_dax->dax_dev))
+		return -ENXIO;
+
+	size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT);
+	ret = -EINVAL;
+	for (i = 0; i < dev_dax->num_resources; i++) {
+		res = &dev_dax->res[i];
+		if (size > resource_size(res)) {
+			dev_info_ratelimited(dev,
+				"%s: %s: fail, vma range overflow\n",
+				current->comm, func);
+			ret = -EINVAL;
+			continue;
+		} else
+			return 0;
+	}
+	return ret;
+}
+
 static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
 		const char *func)
 {
@@ -469,6 +496,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
 	 */
 	id = dax_read_lock();
 	rc = check_vma(dev_dax, vma, __func__);
+	if (!rc)
+		rc = check_vma_range(dev_dax, vma, __func__);
 	dax_read_unlock(id);
 	if (rc)
 		return rc;
-- 
2.7.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-08-13 12:02 [PATCH V2 1/1] device-dax: check for vma range while dax_mmap Zhang Yi
@ 2018-08-20 17:53 ` Verma, Vishal L
  2018-08-20 19:50   ` Dave Jiang
  0 siblings, 1 reply; 7+ messages in thread
From: Verma, Vishal L @ 2018-08-20 17:53 UTC (permalink / raw)
  To: Zhang, Yu C, linux-kernel, yi.z.zhang, Williams, Dan J,
	linux-nvdimm, zwisler, Jiang, Dave, jack
  Cc: Zhang, Yi Z


On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> This patch prevents a user mapping an illegal vma range that is larger
> than a dax device physical resource.
> 
> When qemu maps the dax device for virtual nvdimm's backend device, the
> v-nvdimm label area is defined at the end of mapped range. By using an
> illegal size that exceeds the range of the device dax, it will trigger a
> fault with qemu.
> 
> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> ---
>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

Looks good to me:
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 108c37f..6fe8c30 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -177,6 +177,33 @@ static const struct attribute_group *dax_attribute_groups[] = {
>  	NULL,
>  };
>  
> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma,
> +		const char *func)
> +{
> +	struct device *dev = &dev_dax->dev;
> +	struct resource *res;
> +	unsigned long size;
> +	int ret, i;
> +
> +	if (!dax_alive(dev_dax->dax_dev))
> +		return -ENXIO;
> +
> +	size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT);
> +	ret = -EINVAL;
> +	for (i = 0; i < dev_dax->num_resources; i++) {
> +		res = &dev_dax->res[i];
> +		if (size > resource_size(res)) {
> +			dev_info_ratelimited(dev,
> +				"%s: %s: fail, vma range overflow\n",
> +				current->comm, func);
> +			ret = -EINVAL;
> +			continue;
> +		} else
> +			return 0;
> +	}
> +	return ret;
> +}
> +
>  static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
>  		const char *func)
>  {
> @@ -469,6 +496,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
>  	 */
>  	id = dax_read_lock();
>  	rc = check_vma(dev_dax, vma, __func__);
> +	if (!rc)
> +		rc = check_vma_range(dev_dax, vma, __func__);
>  	dax_read_unlock(id);
>  	if (rc)
>  		return rc;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-08-20 17:53 ` Verma, Vishal L
@ 2018-08-20 19:50   ` Dave Jiang
  2018-08-21 16:16     ` Yi Zhang
  0 siblings, 1 reply; 7+ messages in thread
From: Dave Jiang @ 2018-08-20 19:50 UTC (permalink / raw)
  To: Verma, Vishal L, Zhang, Yu C, linux-kernel, yi.z.zhang, Williams,
	Dan J, linux-nvdimm, zwisler, jack
  Cc: Zhang, Yi Z



On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> 
> On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
>> This patch prevents a user mapping an illegal vma range that is larger
>> than a dax device physical resource.
>>
>> When qemu maps the dax device for virtual nvdimm's backend device, the
>> v-nvdimm label area is defined at the end of mapped range. By using an
>> illegal size that exceeds the range of the device dax, it will trigger a
>> fault with qemu.
>>
>> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
>> ---
>>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
> 
> Looks good to me:
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Applied.

> 
>> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
>> index 108c37f..6fe8c30 100644
>> --- a/drivers/dax/device.c
>> +++ b/drivers/dax/device.c
>> @@ -177,6 +177,33 @@ static const struct attribute_group *dax_attribute_groups[] = {
>>  	NULL,
>>  };
>>  
>> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma,
>> +		const char *func)
>> +{
>> +	struct device *dev = &dev_dax->dev;
>> +	struct resource *res;
>> +	unsigned long size;
>> +	int ret, i;
>> +
>> +	if (!dax_alive(dev_dax->dax_dev))
>> +		return -ENXIO;
>> +
>> +	size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT);
>> +	ret = -EINVAL;
>> +	for (i = 0; i < dev_dax->num_resources; i++) {
>> +		res = &dev_dax->res[i];
>> +		if (size > resource_size(res)) {
>> +			dev_info_ratelimited(dev,
>> +				"%s: %s: fail, vma range overflow\n",
>> +				current->comm, func);
>> +			ret = -EINVAL;
>> +			continue;
>> +		} else
>> +			return 0;
>> +	}
>> +	return ret;
>> +}
>> +
>>  static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
>>  		const char *func)
>>  {
>> @@ -469,6 +496,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
>>  	 */
>>  	id = dax_read_lock();
>>  	rc = check_vma(dev_dax, vma, __func__);
>> +	if (!rc)
>> +		rc = check_vma_range(dev_dax, vma, __func__);
>>  	dax_read_unlock(id);
>>  	if (rc)
>>  		return rc;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-08-20 19:50   ` Dave Jiang
@ 2018-08-21 16:16     ` Yi Zhang
  2018-12-11  0:10       ` Dan Williams
  0 siblings, 1 reply; 7+ messages in thread
From: Yi Zhang @ 2018-08-21 16:16 UTC (permalink / raw)
  To: Dave Jiang; +Cc: jack, Zhang, Yu C, linux-nvdimm, linux-kernel, zwisler

On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> 
> 
> On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > 
> > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> >> This patch prevents a user mapping an illegal vma range that is larger
> >> than a dax device physical resource.
> >>
> >> When qemu maps the dax device for virtual nvdimm's backend device, the
> >> v-nvdimm label area is defined at the end of mapped range. By using an
> >> illegal size that exceeds the range of the device dax, it will trigger a
> >> fault with qemu.
> >>
> >> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> >> ---
> >>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> > 
> > Looks good to me:
> > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> 
> Applied.
Thanks Dava and Vishal's kindly review. Thank you.
> 
> > 
> >> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> >> index 108c37f..6fe8c30 100644
> >> --- a/drivers/dax/device.c
> >> +++ b/drivers/dax/device.c
> >> @@ -177,6 +177,33 @@ static const struct attribute_group *dax_attribute_groups[] = {
> >>  	NULL,
> >>  };
> >>  
> >> +static int check_vma_range(struct dev_dax *dev_dax, struct vm_area_struct *vma,
> >> +		const char *func)
> >> +{
> >> +	struct device *dev = &dev_dax->dev;
> >> +	struct resource *res;
> >> +	unsigned long size;
> >> +	int ret, i;
> >> +
> >> +	if (!dax_alive(dev_dax->dax_dev))
> >> +		return -ENXIO;
> >> +
> >> +	size = vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT);
> >> +	ret = -EINVAL;
> >> +	for (i = 0; i < dev_dax->num_resources; i++) {
> >> +		res = &dev_dax->res[i];
> >> +		if (size > resource_size(res)) {
> >> +			dev_info_ratelimited(dev,
> >> +				"%s: %s: fail, vma range overflow\n",
> >> +				current->comm, func);
> >> +			ret = -EINVAL;
> >> +			continue;
> >> +		} else
> >> +			return 0;
> >> +	}
> >> +	return ret;
> >> +}
> >> +
> >>  static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
> >>  		const char *func)
> >>  {
> >> @@ -469,6 +496,8 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
> >>  	 */
> >>  	id = dax_read_lock();
> >>  	rc = check_vma(dev_dax, vma, __func__);
> >> +	if (!rc)
> >> +		rc = check_vma_range(dev_dax, vma, __func__);
> >>  	dax_read_unlock(id);
> >>  	if (rc)
> >>  		return rc;
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-08-21 16:16     ` Yi Zhang
@ 2018-12-11  0:10       ` Dan Williams
  2018-12-13  6:12         ` Yi Zhang
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Williams @ 2018-12-11  0:10 UTC (permalink / raw)
  To: Dave Jiang, Vishal L Verma, Zhang, Yu C,
	Linux Kernel Mailing List, linux-nvdimm, zwisler, Jan Kara

On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>
> On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> >
> >
> > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > >
> > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > >> This patch prevents a user mapping an illegal vma range that is larger
> > >> than a dax device physical resource.
> > >>
> > >> When qemu maps the dax device for virtual nvdimm's backend device, the
> > >> v-nvdimm label area is defined at the end of mapped range. By using an
> > >> illegal size that exceeds the range of the device dax, it will trigger a
> > >> fault with qemu.
> > >>
> > >> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> > >> ---
> > >>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
> > >>  1 file changed, 29 insertions(+)
> > >>
> > >
> > > Looks good to me:
> > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> >
> > Applied.
> Thanks Dava and Vishal's kindly review. Thank you.

So, it turns out this patch did not get merged for 4.20. I fumbled it
when returning from vacation. However, I'm not sure it is needed. As
long as attempts to access the out-of-range capacity results in SIGBUS
then the implementation is correct. This is similar to the case where
a file is truncated after the vma is established. That size is
validated at fault time.

Could you be clearer about why this is a problem? The fault sounds
like the correct result.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-12-11  0:10       ` Dan Williams
@ 2018-12-13  6:12         ` Yi Zhang
  2018-12-20  1:41           ` Dan Williams
  0 siblings, 1 reply; 7+ messages in thread
From: Yi Zhang @ 2018-12-13  6:12 UTC (permalink / raw)
  To: Dan Williams
  Cc: Zhang, Yu C, linux-nvdimm, Linux Kernel Mailing List, zwisler,
	Jan Kara, Zhang, Yi Z

On 2018-12-10 at 16:10:31 -0800, Dan Williams wrote:
> On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
> >
> > On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> > >
> > >
> > > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > > >
> > > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > > >> This patch prevents a user mapping an illegal vma range that is larger
> > > >> than a dax device physical resource.
> > > >>
> > > >> When qemu maps the dax device for virtual nvdimm's backend device, the
> > > >> v-nvdimm label area is defined at the end of mapped range. By using an
> > > >> illegal size that exceeds the range of the device dax, it will trigger a
> > > >> fault with qemu.
> > > >>
> > > >> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> > > >> ---
> > > >>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
> > > >>  1 file changed, 29 insertions(+)
> > > >>
> > > >
> > > > Looks good to me:
> > > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > >
> > > Applied.
> > Thanks Dava and Vishal's kindly review. Thank you.
> 
> So, it turns out this patch did not get merged for 4.20. I fumbled it
> when returning from vacation. However, I'm not sure it is needed. As
> long as attempts to access the out-of-range capacity results in SIGBUS
> then the implementation is correct. This is similar to the case where
> a file is truncated after the vma is established. That size is
> validated at fault time.
The problem is that we didn't get the fault at we initial the mapping
until attempt to access it, then qemu will failed unexpect without any
output, I think is is better to mention user that we are starting at a 
illegal size, but not faulting at an uncertained time.
> 
> Could you be clearer about why this is a problem? The fault sounds
> like the correct result.
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2 1/1] device-dax: check for vma range while dax_mmap.
  2018-12-13  6:12         ` Yi Zhang
@ 2018-12-20  1:41           ` Dan Williams
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2018-12-20  1:41 UTC (permalink / raw)
  To: Dan Williams, Dave Jiang, Vishal L Verma, Zhang, Yu C,
	Linux Kernel Mailing List, linux-nvdimm, zwisler, Jan Kara

On Wed, Dec 12, 2018 at 10:12 PM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
>
> On 2018-12-10 at 16:10:31 -0800, Dan Williams wrote:
> > On Tue, Aug 21, 2018 at 12:38 AM Yi Zhang <yi.z.zhang@linux.intel.com> wrote:
> > >
> > > On 2018-08-20 at 12:50:31 -0700, Dave Jiang wrote:
> > > >
> > > >
> > > > On 08/20/2018 10:53 AM, Verma, Vishal L wrote:
> > > > >
> > > > > On Mon, 2018-08-13 at 20:02 +0800, Zhang Yi wrote:
> > > > >> This patch prevents a user mapping an illegal vma range that is larger
> > > > >> than a dax device physical resource.
> > > > >>
> > > > >> When qemu maps the dax device for virtual nvdimm's backend device, the
> > > > >> v-nvdimm label area is defined at the end of mapped range. By using an
> > > > >> illegal size that exceeds the range of the device dax, it will trigger a
> > > > >> fault with qemu.
> > > > >>
> > > > >> Signed-off-by: Zhang Yi <yi.z.zhang@linux.intel.com>
> > > > >> ---
> > > > >>  drivers/dax/device.c | 29 +++++++++++++++++++++++++++++
> > > > >>  1 file changed, 29 insertions(+)
> > > > >>
> > > > >
> > > > > Looks good to me:
> > > > > Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> > > >
> > > > Applied.
> > > Thanks Dava and Vishal's kindly review. Thank you.
> >
> > So, it turns out this patch did not get merged for 4.20. I fumbled it
> > when returning from vacation. However, I'm not sure it is needed. As
> > long as attempts to access the out-of-range capacity results in SIGBUS
> > then the implementation is correct. This is similar to the case where
> > a file is truncated after the vma is established. That size is
> > validated at fault time.
> The problem is that we didn't get the fault at we initial the mapping
> until attempt to access it, then qemu will failed unexpect without any
> output, I think is is better to mention user that we are starting at a
> illegal size, but not faulting at an uncertained time.

That can always happen with mmap'd files. There is no guarantee that a
file range an application successfully mmap'd can be faulted in
without triggering a SIGBUS later. So this change would make
device-dax semantics stricter than regular file semantics. For example
the following program prints "map: pass" and then terminates with
SIGBUS. The "test_data" file is a zero sized file.

int main(void)
{
        int fd = open("test_data", O_RDWR);
        void *addr;

        if (fd < 0)
                return -1;

        addr = mmap(NULL, 1 << 20, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
        printf("map: %s\n", addr == MAP_FAILED ? "fail" : "pass");

        *(char *) addr = 0;

        return 0;
}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-12-20  1:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-13 12:02 [PATCH V2 1/1] device-dax: check for vma range while dax_mmap Zhang Yi
2018-08-20 17:53 ` Verma, Vishal L
2018-08-20 19:50   ` Dave Jiang
2018-08-21 16:16     ` Yi Zhang
2018-12-11  0:10       ` Dan Williams
2018-12-13  6:12         ` Yi Zhang
2018-12-20  1:41           ` Dan Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).