| Message ID | 46441800c43f029757c70d8386e3112701081503.1534160958.git.yi.z.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/1] device-dax: check for vma range while dax_mmap. | expand |
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;
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;
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;
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.
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
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; }
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;
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(+)