* [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 7:36 Xie Yongji
2022-04-26 8:07 ` Greg KH
2022-06-02 4:55 ` Yongji Xie
0 siblings, 2 replies; 30+ messages in thread
From: Xie Yongji @ 2022-04-26 7:36 UTC (permalink / raw)
To: mst, jasowang, gregkh; +Cc: mcgrof, akpm, oliver.sang, virtualization, stable
The control device has no drvdata. So we will get a
NULL pointer dereference when accessing control
device's msg_timeout attribute via sysfs:
[ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
[ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
[ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
[ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
[ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
[ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
[ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
[ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
[ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
[ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
[ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
[ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
[ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
[ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
[ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
[ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
[ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
To fix it, don't create the unneeded attribute for
control device anymore.
Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
Reported-by: kernel test robot <oliver.sang@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index f85d1a08ed87..160e40d03084 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
dev->minor = ret;
dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
- dev->dev = device_create(vduse_class, NULL,
- MKDEV(MAJOR(vduse_major), dev->minor),
- dev, "%s", config->name);
+ dev->dev = device_create_with_groups(vduse_class, NULL,
+ MKDEV(MAJOR(vduse_major), dev->minor),
+ dev, vduse_dev_groups, "%s", config->name);
if (IS_ERR(dev->dev)) {
ret = PTR_ERR(dev->dev);
goto err_dev;
@@ -1595,7 +1595,6 @@ static int vduse_init(void)
return PTR_ERR(vduse_class);
vduse_class->devnode = vduse_devnode;
- vduse_class->dev_groups = vduse_dev_groups;
ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
if (ret)
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 7:36 [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access Xie Yongji
@ 2022-04-26 8:07 ` Greg KH
2022-06-02 4:55 ` Yongji Xie
1 sibling, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 8:07 UTC (permalink / raw)
To: Xie Yongji
Cc: mst, jasowang, mcgrof, akpm, oliver.sang, virtualization, stable
On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> The control device has no drvdata. So we will get a
> NULL pointer dereference when accessing control
> device's msg_timeout attribute via sysfs:
>
> [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
>
> To fix it, don't create the unneeded attribute for
> control device anymore.
>
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f85d1a08ed87..160e40d03084 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>
> dev->minor = ret;
> dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> - dev->dev = device_create(vduse_class, NULL,
> - MKDEV(MAJOR(vduse_major), dev->minor),
> - dev, "%s", config->name);
> + dev->dev = device_create_with_groups(vduse_class, NULL,
> + MKDEV(MAJOR(vduse_major), dev->minor),
> + dev, vduse_dev_groups, "%s", config->name);
> if (IS_ERR(dev->dev)) {
> ret = PTR_ERR(dev->dev);
> goto err_dev;
> @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> return PTR_ERR(vduse_class);
>
> vduse_class->devnode = vduse_devnode;
> - vduse_class->dev_groups = vduse_dev_groups;
Ok, this looks much better.
But wow, there are some problems in this code overall. I see a number
of flat-out-wrong things in there that should have been caught by code
reviews. Some examples:
- empty release() callbacks. That is a huge sign the code
design is wrong and broken and you are just trying to make the
driver core quiet for some reason. The documentation in the
kernel explains why this is not ok.
- __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
what you think it does. Please never ever ever do that. It
too is a sign of a broken design.
- no Documentation/ABI/ entries for the sysfs files here. I
think it's burried in some other documentation file but that's
not the correct place for it and if you run scripts/get_abi.pl
with the code loaded it will rightly complain about this.
Do you want to address these, or do you want patches for them?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 8:07 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 8:07 UTC (permalink / raw)
To: Xie Yongji; +Cc: mst, stable, virtualization, mcgrof, oliver.sang, akpm
On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> The control device has no drvdata. So we will get a
> NULL pointer dereference when accessing control
> device's msg_timeout attribute via sysfs:
>
> [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
>
> To fix it, don't create the unneeded attribute for
> control device anymore.
>
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f85d1a08ed87..160e40d03084 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>
> dev->minor = ret;
> dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> - dev->dev = device_create(vduse_class, NULL,
> - MKDEV(MAJOR(vduse_major), dev->minor),
> - dev, "%s", config->name);
> + dev->dev = device_create_with_groups(vduse_class, NULL,
> + MKDEV(MAJOR(vduse_major), dev->minor),
> + dev, vduse_dev_groups, "%s", config->name);
> if (IS_ERR(dev->dev)) {
> ret = PTR_ERR(dev->dev);
> goto err_dev;
> @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> return PTR_ERR(vduse_class);
>
> vduse_class->devnode = vduse_devnode;
> - vduse_class->dev_groups = vduse_dev_groups;
Ok, this looks much better.
But wow, there are some problems in this code overall. I see a number
of flat-out-wrong things in there that should have been caught by code
reviews. Some examples:
- empty release() callbacks. That is a huge sign the code
design is wrong and broken and you are just trying to make the
driver core quiet for some reason. The documentation in the
kernel explains why this is not ok.
- __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
what you think it does. Please never ever ever do that. It
too is a sign of a broken design.
- no Documentation/ABI/ entries for the sysfs files here. I
think it's burried in some other documentation file but that's
not the correct place for it and if you run scripts/get_abi.pl
with the code loaded it will rightly complain about this.
Do you want to address these, or do you want patches for them?
thanks,
greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 8:07 ` Greg KH
(?)
@ 2022-04-26 9:41 ` Yongji Xie
2022-04-26 10:26 ` Greg KH
-1 siblings, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 9:41 UTC (permalink / raw)
To: Greg KH
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > - MKDEV(MAJOR(vduse_major), dev->minor),
> > - dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
>
> Ok, this looks much better.
>
> But wow, there are some problems in this code overall. I see a number
> of flat-out-wrong things in there that should have been caught by code
> reviews. Some examples:
> - empty release() callbacks. That is a huge sign the code
> design is wrong and broken and you are just trying to make the
> driver core quiet for some reason. The documentation in the
> kernel explains why this is not ok.
Sorry, I failed to find the documentation. Do you mean we should
remove the empty release() callbacks?
> - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> what you think it does. Please never ever ever do that. It
> too is a sign of a broken design.
I don't find a good way to remove it. We have to make sure the module
can't be removed until all vduse devices are destroyed.
And I think __module_get(THIS_MODULE) should be safe in our case since
we always call it when we have a reference from open().
> - no Documentation/ABI/ entries for the sysfs files here. I
> think it's burried in some other documentation file but that's
> not the correct place for it and if you run scripts/get_abi.pl
> with the code loaded it will rightly complain about this.
>
OK, I will add one.
> Do you want to address these, or do you want patches for them?
>
Let me send some individual patches for them.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 9:41 ` Yongji Xie
@ 2022-04-26 10:26 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 10:26 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > The control device has no drvdata. So we will get a
> > > NULL pointer dereference when accessing control
> > > device's msg_timeout attribute via sysfs:
> > >
> > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > >
> > > To fix it, don't create the unneeded attribute for
> > > control device anymore.
> > >
> > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index f85d1a08ed87..160e40d03084 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > >
> > > dev->minor = ret;
> > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > - dev->dev = device_create(vduse_class, NULL,
> > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > - dev, "%s", config->name);
> > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > + dev, vduse_dev_groups, "%s", config->name);
> > > if (IS_ERR(dev->dev)) {
> > > ret = PTR_ERR(dev->dev);
> > > goto err_dev;
> > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > return PTR_ERR(vduse_class);
> > >
> > > vduse_class->devnode = vduse_devnode;
> > > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > Ok, this looks much better.
> >
> > But wow, there are some problems in this code overall. I see a number
> > of flat-out-wrong things in there that should have been caught by code
> > reviews. Some examples:
> > - empty release() callbacks. That is a huge sign the code
> > design is wrong and broken and you are just trying to make the
> > driver core quiet for some reason. The documentation in the
> > kernel explains why this is not ok.
>
> Sorry, I failed to find the documentation. Do you mean we should
> remove the empty release() callbacks?
Yes, why are they needed?
(hint, retorical question, you added them to remove the driver core
warning when the device is removed, which means someone added them just
because they thought that their code could ignore the hints that the
driver core was telling them.)
Please properly free the memory here.
> > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > what you think it does. Please never ever ever do that. It
> > too is a sign of a broken design.
>
> I don't find a good way to remove it. We have to make sure the module
> can't be removed until all vduse devices are destroyed.
That will happen automatically when the module is removed.
> And I think __module_get(THIS_MODULE) should be safe in our case since
> we always call it when we have a reference from open().
What happened if someone removed the module _right before_ this was
called? You can not grab your own reference count safely.
Please just remove it, it's not needed and is broken. There should not
be any reason that the module can not be unloaded, UNLESS a file handle
is open, and you properly handle that already.
thanks,
greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 10:26 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 10:26 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > The control device has no drvdata. So we will get a
> > > NULL pointer dereference when accessing control
> > > device's msg_timeout attribute via sysfs:
> > >
> > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > >
> > > To fix it, don't create the unneeded attribute for
> > > control device anymore.
> > >
> > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index f85d1a08ed87..160e40d03084 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > >
> > > dev->minor = ret;
> > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > - dev->dev = device_create(vduse_class, NULL,
> > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > - dev, "%s", config->name);
> > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > + dev, vduse_dev_groups, "%s", config->name);
> > > if (IS_ERR(dev->dev)) {
> > > ret = PTR_ERR(dev->dev);
> > > goto err_dev;
> > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > return PTR_ERR(vduse_class);
> > >
> > > vduse_class->devnode = vduse_devnode;
> > > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > Ok, this looks much better.
> >
> > But wow, there are some problems in this code overall. I see a number
> > of flat-out-wrong things in there that should have been caught by code
> > reviews. Some examples:
> > - empty release() callbacks. That is a huge sign the code
> > design is wrong and broken and you are just trying to make the
> > driver core quiet for some reason. The documentation in the
> > kernel explains why this is not ok.
>
> Sorry, I failed to find the documentation. Do you mean we should
> remove the empty release() callbacks?
Yes, why are they needed?
(hint, retorical question, you added them to remove the driver core
warning when the device is removed, which means someone added them just
because they thought that their code could ignore the hints that the
driver core was telling them.)
Please properly free the memory here.
> > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > what you think it does. Please never ever ever do that. It
> > too is a sign of a broken design.
>
> I don't find a good way to remove it. We have to make sure the module
> can't be removed until all vduse devices are destroyed.
That will happen automatically when the module is removed.
> And I think __module_get(THIS_MODULE) should be safe in our case since
> we always call it when we have a reference from open().
What happened if someone removed the module _right before_ this was
called? You can not grab your own reference count safely.
Please just remove it, it's not needed and is broken. There should not
be any reason that the module can not be unloaded, UNLESS a file handle
is open, and you properly handle that already.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 10:26 ` Greg KH
(?)
@ 2022-04-26 12:30 ` Yongji Xie
2022-04-26 12:44 ` Greg KH
-1 siblings, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 12:30 UTC (permalink / raw)
To: Greg KH
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > The control device has no drvdata. So we will get a
> > > > NULL pointer dereference when accessing control
> > > > device's msg_timeout attribute via sysfs:
> > > >
> > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > >
> > > > To fix it, don't create the unneeded attribute for
> > > > control device anymore.
> > > >
> > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > ---
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index f85d1a08ed87..160e40d03084 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > >
> > > > dev->minor = ret;
> > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > - dev->dev = device_create(vduse_class, NULL,
> > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > - dev, "%s", config->name);
> > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > if (IS_ERR(dev->dev)) {
> > > > ret = PTR_ERR(dev->dev);
> > > > goto err_dev;
> > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > return PTR_ERR(vduse_class);
> > > >
> > > > vduse_class->devnode = vduse_devnode;
> > > > - vduse_class->dev_groups = vduse_dev_groups;
> > >
> > > Ok, this looks much better.
> > >
> > > But wow, there are some problems in this code overall. I see a number
> > > of flat-out-wrong things in there that should have been caught by code
> > > reviews. Some examples:
> > > - empty release() callbacks. That is a huge sign the code
> > > design is wrong and broken and you are just trying to make the
> > > driver core quiet for some reason. The documentation in the
> > > kernel explains why this is not ok.
> >
> > Sorry, I failed to find the documentation. Do you mean we should
> > remove the empty release() callbacks?
>
> Yes, why are they needed?
>
> (hint, retorical question, you added them to remove the driver core
> warning when the device is removed, which means someone added them just
> because they thought that their code could ignore the hints that the
> driver core was telling them.)
>
OK, I see.
> Please properly free the memory here.
>
One question is how to deal with the case if the device/kobject is
defined as a static variable. We should not need to free any resources
in this case. Or do you suggest just using dynamic allocation here?
> > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > what you think it does. Please never ever ever do that. It
> > > too is a sign of a broken design.
> >
> > I don't find a good way to remove it. We have to make sure the module
> > can't be removed until all vduse devices are destroyed.
>
> That will happen automatically when the module is removed.
>
> > And I think __module_get(THIS_MODULE) should be safe in our case since
> > we always call it when we have a reference from open().
>
> What happened if someone removed the module _right before_ this was
> called? You can not grab your own reference count safely.
>
I don't get you here. We should already grab a reference count from
open() before calling this. So it should fail if someone tries to
remove the module at this time.
> Please just remove it, it's not needed and is broken. There should not
> be any reason that the module can not be unloaded, UNLESS a file handle
> is open, and you properly handle that already.
>
But in our case, I think we should prevent unloading the module If we
already created some vduse devices via /dev/vduse/control (note that
the control device's file handle could be closed after device
creation). Otherwise, we might get some crashes when accessing those
created vduse devices.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 12:30 ` Yongji Xie
@ 2022-04-26 12:44 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 12:44 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > The control device has no drvdata. So we will get a
> > > > > NULL pointer dereference when accessing control
> > > > > device's msg_timeout attribute via sysfs:
> > > > >
> > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > >
> > > > > To fix it, don't create the unneeded attribute for
> > > > > control device anymore.
> > > > >
> > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > ---
> > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > >
> > > > > dev->minor = ret;
> > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > - dev, "%s", config->name);
> > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > if (IS_ERR(dev->dev)) {
> > > > > ret = PTR_ERR(dev->dev);
> > > > > goto err_dev;
> > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > return PTR_ERR(vduse_class);
> > > > >
> > > > > vduse_class->devnode = vduse_devnode;
> > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > >
> > > > Ok, this looks much better.
> > > >
> > > > But wow, there are some problems in this code overall. I see a number
> > > > of flat-out-wrong things in there that should have been caught by code
> > > > reviews. Some examples:
> > > > - empty release() callbacks. That is a huge sign the code
> > > > design is wrong and broken and you are just trying to make the
> > > > driver core quiet for some reason. The documentation in the
> > > > kernel explains why this is not ok.
> > >
> > > Sorry, I failed to find the documentation. Do you mean we should
> > > remove the empty release() callbacks?
> >
> > Yes, why are they needed?
> >
> > (hint, retorical question, you added them to remove the driver core
> > warning when the device is removed, which means someone added them just
> > because they thought that their code could ignore the hints that the
> > driver core was telling them.)
> >
>
> OK, I see.
>
> > Please properly free the memory here.
> >
>
> One question is how to deal with the case if the device/kobject is
> defined as a static variable. We should not need to free any resources
> in this case. Or do you suggest just using dynamic allocation here?
A kobject can NEVER be a static variable[1]. That's not how the driver
model works at all. If this is how this code is written, it needs to be
fixed.
[1] Ok, yes, drivers and busses and classes have static kobjects, ignore
them...
>
> > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > what you think it does. Please never ever ever do that. It
> > > > too is a sign of a broken design.
> > >
> > > I don't find a good way to remove it. We have to make sure the module
> > > can't be removed until all vduse devices are destroyed.
> >
> > That will happen automatically when the module is removed.
> >
> > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > we always call it when we have a reference from open().
> >
> > What happened if someone removed the module _right before_ this was
> > called? You can not grab your own reference count safely.
> >
>
> I don't get you here. We should already grab a reference count from
> open() before calling this. So it should fail if someone tries to
> remove the module at this time.
Then why are you trying to grab the module reference again?
> > Please just remove it, it's not needed and is broken. There should not
> > be any reason that the module can not be unloaded, UNLESS a file handle
> > is open, and you properly handle that already.
> >
>
> But in our case, I think we should prevent unloading the module If we
> already created some vduse devices via /dev/vduse/control (note that
> the control device's file handle could be closed after device
> creation). Otherwise, we might get some crashes when accessing those
> created vduse devices.
Then the code is written incorrectly, this should not be an issue at
all. Your devices will all be cleaned up properly before your code is
unloaded from the system.
Note that no other driver or bus does this, what makes this different?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 12:44 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 12:44 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > The control device has no drvdata. So we will get a
> > > > > NULL pointer dereference when accessing control
> > > > > device's msg_timeout attribute via sysfs:
> > > > >
> > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > >
> > > > > To fix it, don't create the unneeded attribute for
> > > > > control device anymore.
> > > > >
> > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > ---
> > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > >
> > > > > dev->minor = ret;
> > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > - dev, "%s", config->name);
> > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > if (IS_ERR(dev->dev)) {
> > > > > ret = PTR_ERR(dev->dev);
> > > > > goto err_dev;
> > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > return PTR_ERR(vduse_class);
> > > > >
> > > > > vduse_class->devnode = vduse_devnode;
> > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > >
> > > > Ok, this looks much better.
> > > >
> > > > But wow, there are some problems in this code overall. I see a number
> > > > of flat-out-wrong things in there that should have been caught by code
> > > > reviews. Some examples:
> > > > - empty release() callbacks. That is a huge sign the code
> > > > design is wrong and broken and you are just trying to make the
> > > > driver core quiet for some reason. The documentation in the
> > > > kernel explains why this is not ok.
> > >
> > > Sorry, I failed to find the documentation. Do you mean we should
> > > remove the empty release() callbacks?
> >
> > Yes, why are they needed?
> >
> > (hint, retorical question, you added them to remove the driver core
> > warning when the device is removed, which means someone added them just
> > because they thought that their code could ignore the hints that the
> > driver core was telling them.)
> >
>
> OK, I see.
>
> > Please properly free the memory here.
> >
>
> One question is how to deal with the case if the device/kobject is
> defined as a static variable. We should not need to free any resources
> in this case. Or do you suggest just using dynamic allocation here?
A kobject can NEVER be a static variable[1]. That's not how the driver
model works at all. If this is how this code is written, it needs to be
fixed.
[1] Ok, yes, drivers and busses and classes have static kobjects, ignore
them...
>
> > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > what you think it does. Please never ever ever do that. It
> > > > too is a sign of a broken design.
> > >
> > > I don't find a good way to remove it. We have to make sure the module
> > > can't be removed until all vduse devices are destroyed.
> >
> > That will happen automatically when the module is removed.
> >
> > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > we always call it when we have a reference from open().
> >
> > What happened if someone removed the module _right before_ this was
> > called? You can not grab your own reference count safely.
> >
>
> I don't get you here. We should already grab a reference count from
> open() before calling this. So it should fail if someone tries to
> remove the module at this time.
Then why are you trying to grab the module reference again?
> > Please just remove it, it's not needed and is broken. There should not
> > be any reason that the module can not be unloaded, UNLESS a file handle
> > is open, and you properly handle that already.
> >
>
> But in our case, I think we should prevent unloading the module If we
> already created some vduse devices via /dev/vduse/control (note that
> the control device's file handle could be closed after device
> creation). Otherwise, we might get some crashes when accessing those
> created vduse devices.
Then the code is written incorrectly, this should not be an issue at
all. Your devices will all be cleaned up properly before your code is
unloaded from the system.
Note that no other driver or bus does this, what makes this different?
thanks,
greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 12:44 ` Greg KH
(?)
@ 2022-04-26 13:17 ` Yongji Xie
2022-04-26 13:34 ` Greg KH
-1 siblings, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 13:17 UTC (permalink / raw)
To: Greg KH
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > The control device has no drvdata. So we will get a
> > > > > > NULL pointer dereference when accessing control
> > > > > > device's msg_timeout attribute via sysfs:
> > > > > >
> > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > >
> > > > > > To fix it, don't create the unneeded attribute for
> > > > > > control device anymore.
> > > > > >
> > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > ---
> > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > >
> > > > > > dev->minor = ret;
> > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > - dev, "%s", config->name);
> > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > if (IS_ERR(dev->dev)) {
> > > > > > ret = PTR_ERR(dev->dev);
> > > > > > goto err_dev;
> > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > return PTR_ERR(vduse_class);
> > > > > >
> > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > >
> > > > > Ok, this looks much better.
> > > > >
> > > > > But wow, there are some problems in this code overall. I see a number
> > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > reviews. Some examples:
> > > > > - empty release() callbacks. That is a huge sign the code
> > > > > design is wrong and broken and you are just trying to make the
> > > > > driver core quiet for some reason. The documentation in the
> > > > > kernel explains why this is not ok.
> > > >
> > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > remove the empty release() callbacks?
> > >
> > > Yes, why are they needed?
> > >
> > > (hint, retorical question, you added them to remove the driver core
> > > warning when the device is removed, which means someone added them just
> > > because they thought that their code could ignore the hints that the
> > > driver core was telling them.)
> > >
> >
> > OK, I see.
> >
> > > Please properly free the memory here.
> > >
> >
> > One question is how to deal with the case if the device/kobject is
> > defined as a static variable. We should not need to free any resources
> > in this case. Or do you suggest just using dynamic allocation here?
>
> A kobject can NEVER be a static variable[1]. That's not how the driver
> model works at all. If this is how this code is written, it needs to be
> fixed.
>
OK, I see.
> [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> them...
>
> >
> > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > what you think it does. Please never ever ever do that. It
> > > > > too is a sign of a broken design.
> > > >
> > > > I don't find a good way to remove it. We have to make sure the module
> > > > can't be removed until all vduse devices are destroyed.
> > >
> > > That will happen automatically when the module is removed.
> > >
> > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > we always call it when we have a reference from open().
> > >
> > > What happened if someone removed the module _right before_ this was
> > > called? You can not grab your own reference count safely.
> > >
> >
> > I don't get you here. We should already grab a reference count from
> > open() before calling this. So it should fail if someone tries to
> > remove the module at this time.
>
> Then why are you trying to grab the module reference again?
>
> > > Please just remove it, it's not needed and is broken. There should not
> > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > is open, and you properly handle that already.
> > >
> >
> > But in our case, I think we should prevent unloading the module If we
> > already created some vduse devices via /dev/vduse/control (note that
> > the control device's file handle could be closed after device
> > creation). Otherwise, we might get some crashes when accessing those
> > created vduse devices.
>
> Then the code is written incorrectly, this should not be an issue at
> all. Your devices will all be cleaned up properly before your code is
> unloaded from the system.
>
In current design, the vduse device can't be cleaned up properly until
it is unbinded from the vDPA bus explicitly. So I use the extra
__module_get() to make sure we can't unload the module until the
device is cleaned up properly.
> Note that no other driver or bus does this, what makes this different?
>
I can see some similar behavior in loop and rbd modules.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 13:17 ` Yongji Xie
@ 2022-04-26 13:34 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 13:34 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > The control device has no drvdata. So we will get a
> > > > > > > NULL pointer dereference when accessing control
> > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > >
> > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > >
> > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > control device anymore.
> > > > > > >
> > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > ---
> > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > >
> > > > > > > dev->minor = ret;
> > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > - dev, "%s", config->name);
> > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > goto err_dev;
> > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > return PTR_ERR(vduse_class);
> > > > > > >
> > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > >
> > > > > > Ok, this looks much better.
> > > > > >
> > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > reviews. Some examples:
> > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > design is wrong and broken and you are just trying to make the
> > > > > > driver core quiet for some reason. The documentation in the
> > > > > > kernel explains why this is not ok.
> > > > >
> > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > remove the empty release() callbacks?
> > > >
> > > > Yes, why are they needed?
> > > >
> > > > (hint, retorical question, you added them to remove the driver core
> > > > warning when the device is removed, which means someone added them just
> > > > because they thought that their code could ignore the hints that the
> > > > driver core was telling them.)
> > > >
> > >
> > > OK, I see.
> > >
> > > > Please properly free the memory here.
> > > >
> > >
> > > One question is how to deal with the case if the device/kobject is
> > > defined as a static variable. We should not need to free any resources
> > > in this case. Or do you suggest just using dynamic allocation here?
> >
> > A kobject can NEVER be a static variable[1]. That's not how the driver
> > model works at all. If this is how this code is written, it needs to be
> > fixed.
> >
>
> OK, I see.
>
> > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > them...
> >
> > >
> > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > what you think it does. Please never ever ever do that. It
> > > > > > too is a sign of a broken design.
> > > > >
> > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > can't be removed until all vduse devices are destroyed.
> > > >
> > > > That will happen automatically when the module is removed.
> > > >
> > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > we always call it when we have a reference from open().
> > > >
> > > > What happened if someone removed the module _right before_ this was
> > > > called? You can not grab your own reference count safely.
> > > >
> > >
> > > I don't get you here. We should already grab a reference count from
> > > open() before calling this. So it should fail if someone tries to
> > > remove the module at this time.
> >
> > Then why are you trying to grab the module reference again?
> >
> > > > Please just remove it, it's not needed and is broken. There should not
> > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > is open, and you properly handle that already.
> > > >
> > >
> > > But in our case, I think we should prevent unloading the module If we
> > > already created some vduse devices via /dev/vduse/control (note that
> > > the control device's file handle could be closed after device
> > > creation). Otherwise, we might get some crashes when accessing those
> > > created vduse devices.
> >
> > Then the code is written incorrectly, this should not be an issue at
> > all. Your devices will all be cleaned up properly before your code is
> > unloaded from the system.
> >
>
> In current design, the vduse device can't be cleaned up properly until
> it is unbinded from the vDPA bus explicitly. So I use the extra
> __module_get() to make sure we can't unload the module until the
> device is cleaned up properly.
Then something is wrong, it should not work that way.
> > Note that no other driver or bus does this, what makes this different?
> >
>
> I can see some similar behavior in loop and rbd modules.
Never treat the loop code as a good example :)
This should not be needed, when your module is unloaded, all devices it
handled should be properly removed by it.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 13:34 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 13:34 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > The control device has no drvdata. So we will get a
> > > > > > > NULL pointer dereference when accessing control
> > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > >
> > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > >
> > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > control device anymore.
> > > > > > >
> > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > ---
> > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > >
> > > > > > > dev->minor = ret;
> > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > - dev, "%s", config->name);
> > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > goto err_dev;
> > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > return PTR_ERR(vduse_class);
> > > > > > >
> > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > >
> > > > > > Ok, this looks much better.
> > > > > >
> > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > reviews. Some examples:
> > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > design is wrong and broken and you are just trying to make the
> > > > > > driver core quiet for some reason. The documentation in the
> > > > > > kernel explains why this is not ok.
> > > > >
> > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > remove the empty release() callbacks?
> > > >
> > > > Yes, why are they needed?
> > > >
> > > > (hint, retorical question, you added them to remove the driver core
> > > > warning when the device is removed, which means someone added them just
> > > > because they thought that their code could ignore the hints that the
> > > > driver core was telling them.)
> > > >
> > >
> > > OK, I see.
> > >
> > > > Please properly free the memory here.
> > > >
> > >
> > > One question is how to deal with the case if the device/kobject is
> > > defined as a static variable. We should not need to free any resources
> > > in this case. Or do you suggest just using dynamic allocation here?
> >
> > A kobject can NEVER be a static variable[1]. That's not how the driver
> > model works at all. If this is how this code is written, it needs to be
> > fixed.
> >
>
> OK, I see.
>
> > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > them...
> >
> > >
> > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > what you think it does. Please never ever ever do that. It
> > > > > > too is a sign of a broken design.
> > > > >
> > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > can't be removed until all vduse devices are destroyed.
> > > >
> > > > That will happen automatically when the module is removed.
> > > >
> > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > we always call it when we have a reference from open().
> > > >
> > > > What happened if someone removed the module _right before_ this was
> > > > called? You can not grab your own reference count safely.
> > > >
> > >
> > > I don't get you here. We should already grab a reference count from
> > > open() before calling this. So it should fail if someone tries to
> > > remove the module at this time.
> >
> > Then why are you trying to grab the module reference again?
> >
> > > > Please just remove it, it's not needed and is broken. There should not
> > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > is open, and you properly handle that already.
> > > >
> > >
> > > But in our case, I think we should prevent unloading the module If we
> > > already created some vduse devices via /dev/vduse/control (note that
> > > the control device's file handle could be closed after device
> > > creation). Otherwise, we might get some crashes when accessing those
> > > created vduse devices.
> >
> > Then the code is written incorrectly, this should not be an issue at
> > all. Your devices will all be cleaned up properly before your code is
> > unloaded from the system.
> >
>
> In current design, the vduse device can't be cleaned up properly until
> it is unbinded from the vDPA bus explicitly. So I use the extra
> __module_get() to make sure we can't unload the module until the
> device is cleaned up properly.
Then something is wrong, it should not work that way.
> > Note that no other driver or bus does this, what makes this different?
> >
>
> I can see some similar behavior in loop and rbd modules.
Never treat the loop code as a good example :)
This should not be needed, when your module is unloaded, all devices it
handled should be properly removed by it.
thanks,
greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 13:34 ` Greg KH
(?)
@ 2022-04-26 14:02 ` Yongji Xie
2022-04-26 14:09 ` Greg KH
2022-04-26 14:17 ` Michael S. Tsirkin
-1 siblings, 2 replies; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 14:02 UTC (permalink / raw)
To: Greg KH
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 9:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> > On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > > The control device has no drvdata. So we will get a
> > > > > > > > NULL pointer dereference when accessing control
> > > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > > >
> > > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > > >
> > > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > > control device anymore.
> > > > > > > >
> > > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > > ---
> > > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > > >
> > > > > > > > dev->minor = ret;
> > > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > - dev, "%s", config->name);
> > > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > > goto err_dev;
> > > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > > return PTR_ERR(vduse_class);
> > > > > > > >
> > > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > > >
> > > > > > > Ok, this looks much better.
> > > > > > >
> > > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > > reviews. Some examples:
> > > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > > design is wrong and broken and you are just trying to make the
> > > > > > > driver core quiet for some reason. The documentation in the
> > > > > > > kernel explains why this is not ok.
> > > > > >
> > > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > > remove the empty release() callbacks?
> > > > >
> > > > > Yes, why are they needed?
> > > > >
> > > > > (hint, retorical question, you added them to remove the driver core
> > > > > warning when the device is removed, which means someone added them just
> > > > > because they thought that their code could ignore the hints that the
> > > > > driver core was telling them.)
> > > > >
> > > >
> > > > OK, I see.
> > > >
> > > > > Please properly free the memory here.
> > > > >
> > > >
> > > > One question is how to deal with the case if the device/kobject is
> > > > defined as a static variable. We should not need to free any resources
> > > > in this case. Or do you suggest just using dynamic allocation here?
> > >
> > > A kobject can NEVER be a static variable[1]. That's not how the driver
> > > model works at all. If this is how this code is written, it needs to be
> > > fixed.
> > >
> >
> > OK, I see.
> >
> > > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > > them...
> > >
> > > >
> > > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > > what you think it does. Please never ever ever do that. It
> > > > > > > too is a sign of a broken design.
> > > > > >
> > > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > > can't be removed until all vduse devices are destroyed.
> > > > >
> > > > > That will happen automatically when the module is removed.
> > > > >
> > > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > > we always call it when we have a reference from open().
> > > > >
> > > > > What happened if someone removed the module _right before_ this was
> > > > > called? You can not grab your own reference count safely.
> > > > >
> > > >
> > > > I don't get you here. We should already grab a reference count from
> > > > open() before calling this. So it should fail if someone tries to
> > > > remove the module at this time.
> > >
> > > Then why are you trying to grab the module reference again?
> > >
> > > > > Please just remove it, it's not needed and is broken. There should not
> > > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > > is open, and you properly handle that already.
> > > > >
> > > >
> > > > But in our case, I think we should prevent unloading the module If we
> > > > already created some vduse devices via /dev/vduse/control (note that
> > > > the control device's file handle could be closed after device
> > > > creation). Otherwise, we might get some crashes when accessing those
> > > > created vduse devices.
> > >
> > > Then the code is written incorrectly, this should not be an issue at
> > > all. Your devices will all be cleaned up properly before your code is
> > > unloaded from the system.
> > >
> >
> > In current design, the vduse device can't be cleaned up properly until
> > it is unbinded from the vDPA bus explicitly. So I use the extra
> > __module_get() to make sure we can't unload the module until the
> > device is cleaned up properly.
>
> Then something is wrong, it should not work that way.
>
> > > Note that no other driver or bus does this, what makes this different?
> > >
> >
> > I can see some similar behavior in loop and rbd modules.
>
> Never treat the loop code as a good example :)
>
OK.
> This should not be needed, when your module is unloaded, all devices it
> handled should be properly removed by it.
>
I see. But it's not easy to achieve that currently. Maybe we need
something like DEVICE_NEEDS_RESET support in virtio core.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:02 ` Yongji Xie
@ 2022-04-26 14:09 ` Greg KH
2022-04-26 14:17 ` Michael S. Tsirkin
1 sibling, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 14:09 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 9:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > > > The control device has no drvdata. So we will get a
> > > > > > > > > NULL pointer dereference when accessing control
> > > > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > > > >
> > > > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > > > >
> > > > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > > > control device anymore.
> > > > > > > > >
> > > > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > > > ---
> > > > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > > > >
> > > > > > > > > dev->minor = ret;
> > > > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > - dev, "%s", config->name);
> > > > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > > > goto err_dev;
> > > > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > > > return PTR_ERR(vduse_class);
> > > > > > > > >
> > > > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > > > >
> > > > > > > > Ok, this looks much better.
> > > > > > > >
> > > > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > > > reviews. Some examples:
> > > > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > > > design is wrong and broken and you are just trying to make the
> > > > > > > > driver core quiet for some reason. The documentation in the
> > > > > > > > kernel explains why this is not ok.
> > > > > > >
> > > > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > > > remove the empty release() callbacks?
> > > > > >
> > > > > > Yes, why are they needed?
> > > > > >
> > > > > > (hint, retorical question, you added them to remove the driver core
> > > > > > warning when the device is removed, which means someone added them just
> > > > > > because they thought that their code could ignore the hints that the
> > > > > > driver core was telling them.)
> > > > > >
> > > > >
> > > > > OK, I see.
> > > > >
> > > > > > Please properly free the memory here.
> > > > > >
> > > > >
> > > > > One question is how to deal with the case if the device/kobject is
> > > > > defined as a static variable. We should not need to free any resources
> > > > > in this case. Or do you suggest just using dynamic allocation here?
> > > >
> > > > A kobject can NEVER be a static variable[1]. That's not how the driver
> > > > model works at all. If this is how this code is written, it needs to be
> > > > fixed.
> > > >
> > >
> > > OK, I see.
> > >
> > > > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > > > them...
> > > >
> > > > >
> > > > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > > > what you think it does. Please never ever ever do that. It
> > > > > > > > too is a sign of a broken design.
> > > > > > >
> > > > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > > > can't be removed until all vduse devices are destroyed.
> > > > > >
> > > > > > That will happen automatically when the module is removed.
> > > > > >
> > > > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > > > we always call it when we have a reference from open().
> > > > > >
> > > > > > What happened if someone removed the module _right before_ this was
> > > > > > called? You can not grab your own reference count safely.
> > > > > >
> > > > >
> > > > > I don't get you here. We should already grab a reference count from
> > > > > open() before calling this. So it should fail if someone tries to
> > > > > remove the module at this time.
> > > >
> > > > Then why are you trying to grab the module reference again?
> > > >
> > > > > > Please just remove it, it's not needed and is broken. There should not
> > > > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > > > is open, and you properly handle that already.
> > > > > >
> > > > >
> > > > > But in our case, I think we should prevent unloading the module If we
> > > > > already created some vduse devices via /dev/vduse/control (note that
> > > > > the control device's file handle could be closed after device
> > > > > creation). Otherwise, we might get some crashes when accessing those
> > > > > created vduse devices.
> > > >
> > > > Then the code is written incorrectly, this should not be an issue at
> > > > all. Your devices will all be cleaned up properly before your code is
> > > > unloaded from the system.
> > > >
> > >
> > > In current design, the vduse device can't be cleaned up properly until
> > > it is unbinded from the vDPA bus explicitly. So I use the extra
> > > __module_get() to make sure we can't unload the module until the
> > > device is cleaned up properly.
> >
> > Then something is wrong, it should not work that way.
> >
> > > > Note that no other driver or bus does this, what makes this different?
> > > >
> > >
> > > I can see some similar behavior in loop and rbd modules.
> >
> > Never treat the loop code as a good example :)
> >
>
> OK.
>
> > This should not be needed, when your module is unloaded, all devices it
> > handled should be properly removed by it.
> >
>
> I see. But it's not easy to achieve that currently. Maybe we need
> something like DEVICE_NEEDS_RESET support in virtio core.
This shouldn't be that hard, again, no other subsystem needs this. How
about fix the other issues first and then we can work on that one.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 14:09 ` Greg KH
0 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2022-04-26 14:09 UTC (permalink / raw)
To: Yongji Xie
Cc: Michael S. Tsirkin, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 9:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> > > On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > > > The control device has no drvdata. So we will get a
> > > > > > > > > NULL pointer dereference when accessing control
> > > > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > > > >
> > > > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > > > >
> > > > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > > > control device anymore.
> > > > > > > > >
> > > > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > > > ---
> > > > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > > > >
> > > > > > > > > dev->minor = ret;
> > > > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > - dev, "%s", config->name);
> > > > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > > > goto err_dev;
> > > > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > > > return PTR_ERR(vduse_class);
> > > > > > > > >
> > > > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > > > >
> > > > > > > > Ok, this looks much better.
> > > > > > > >
> > > > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > > > reviews. Some examples:
> > > > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > > > design is wrong and broken and you are just trying to make the
> > > > > > > > driver core quiet for some reason. The documentation in the
> > > > > > > > kernel explains why this is not ok.
> > > > > > >
> > > > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > > > remove the empty release() callbacks?
> > > > > >
> > > > > > Yes, why are they needed?
> > > > > >
> > > > > > (hint, retorical question, you added them to remove the driver core
> > > > > > warning when the device is removed, which means someone added them just
> > > > > > because they thought that their code could ignore the hints that the
> > > > > > driver core was telling them.)
> > > > > >
> > > > >
> > > > > OK, I see.
> > > > >
> > > > > > Please properly free the memory here.
> > > > > >
> > > > >
> > > > > One question is how to deal with the case if the device/kobject is
> > > > > defined as a static variable. We should not need to free any resources
> > > > > in this case. Or do you suggest just using dynamic allocation here?
> > > >
> > > > A kobject can NEVER be a static variable[1]. That's not how the driver
> > > > model works at all. If this is how this code is written, it needs to be
> > > > fixed.
> > > >
> > >
> > > OK, I see.
> > >
> > > > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > > > them...
> > > >
> > > > >
> > > > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > > > what you think it does. Please never ever ever do that. It
> > > > > > > > too is a sign of a broken design.
> > > > > > >
> > > > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > > > can't be removed until all vduse devices are destroyed.
> > > > > >
> > > > > > That will happen automatically when the module is removed.
> > > > > >
> > > > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > > > we always call it when we have a reference from open().
> > > > > >
> > > > > > What happened if someone removed the module _right before_ this was
> > > > > > called? You can not grab your own reference count safely.
> > > > > >
> > > > >
> > > > > I don't get you here. We should already grab a reference count from
> > > > > open() before calling this. So it should fail if someone tries to
> > > > > remove the module at this time.
> > > >
> > > > Then why are you trying to grab the module reference again?
> > > >
> > > > > > Please just remove it, it's not needed and is broken. There should not
> > > > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > > > is open, and you properly handle that already.
> > > > > >
> > > > >
> > > > > But in our case, I think we should prevent unloading the module If we
> > > > > already created some vduse devices via /dev/vduse/control (note that
> > > > > the control device's file handle could be closed after device
> > > > > creation). Otherwise, we might get some crashes when accessing those
> > > > > created vduse devices.
> > > >
> > > > Then the code is written incorrectly, this should not be an issue at
> > > > all. Your devices will all be cleaned up properly before your code is
> > > > unloaded from the system.
> > > >
> > >
> > > In current design, the vduse device can't be cleaned up properly until
> > > it is unbinded from the vDPA bus explicitly. So I use the extra
> > > __module_get() to make sure we can't unload the module until the
> > > device is cleaned up properly.
> >
> > Then something is wrong, it should not work that way.
> >
> > > > Note that no other driver or bus does this, what makes this different?
> > > >
> > >
> > > I can see some similar behavior in loop and rbd modules.
> >
> > Never treat the loop code as a good example :)
> >
>
> OK.
>
> > This should not be needed, when your module is unloaded, all devices it
> > handled should be properly removed by it.
> >
>
> I see. But it's not easy to achieve that currently. Maybe we need
> something like DEVICE_NEEDS_RESET support in virtio core.
This shouldn't be that hard, again, no other subsystem needs this. How
about fix the other issues first and then we can work on that one.
thanks,
greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:09 ` Greg KH
(?)
@ 2022-04-26 14:15 ` Yongji Xie
-1 siblings, 0 replies; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 14:15 UTC (permalink / raw)
To: Greg KH
Cc: Michael S. Tsirkin, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:09 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > On Tue, Apr 26, 2022 at 9:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 09:17:23PM +0800, Yongji Xie wrote:
> > > > On Tue, Apr 26, 2022 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Apr 26, 2022 at 08:30:38PM +0800, Yongji Xie wrote:
> > > > > > On Tue, Apr 26, 2022 at 6:26 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 26, 2022 at 05:41:15PM +0800, Yongji Xie wrote:
> > > > > > > > On Tue, Apr 26, 2022 at 4:07 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > > > > > > > The control device has no drvdata. So we will get a
> > > > > > > > > > NULL pointer dereference when accessing control
> > > > > > > > > > device's msg_timeout attribute via sysfs:
> > > > > > > > > >
> > > > > > > > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > > > > > > > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > > > > > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > > > > > > > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > > > > > > > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > > > > > > > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > > > > > > > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > > > > > > > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > > > > > > > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > > > > > > > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > > > > > > > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > > > > > > > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > > > > > > > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > > > > > > > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > > > > > > > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > > > > > > > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > > > > > > > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > > > > > > > >
> > > > > > > > > > To fix it, don't create the unneeded attribute for
> > > > > > > > > > control device anymore.
> > > > > > > > > >
> > > > > > > > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > > > > > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > > index f85d1a08ed87..160e40d03084 100644
> > > > > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > > > > > > > > >
> > > > > > > > > > dev->minor = ret;
> > > > > > > > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > > > > > > > - dev->dev = device_create(vduse_class, NULL,
> > > > > > > > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > > - dev, "%s", config->name);
> > > > > > > > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > > > > > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > > > > > > > + dev, vduse_dev_groups, "%s", config->name);
> > > > > > > > > > if (IS_ERR(dev->dev)) {
> > > > > > > > > > ret = PTR_ERR(dev->dev);
> > > > > > > > > > goto err_dev;
> > > > > > > > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > > > > > > > return PTR_ERR(vduse_class);
> > > > > > > > > >
> > > > > > > > > > vduse_class->devnode = vduse_devnode;
> > > > > > > > > > - vduse_class->dev_groups = vduse_dev_groups;
> > > > > > > > >
> > > > > > > > > Ok, this looks much better.
> > > > > > > > >
> > > > > > > > > But wow, there are some problems in this code overall. I see a number
> > > > > > > > > of flat-out-wrong things in there that should have been caught by code
> > > > > > > > > reviews. Some examples:
> > > > > > > > > - empty release() callbacks. That is a huge sign the code
> > > > > > > > > design is wrong and broken and you are just trying to make the
> > > > > > > > > driver core quiet for some reason. The documentation in the
> > > > > > > > > kernel explains why this is not ok.
> > > > > > > >
> > > > > > > > Sorry, I failed to find the documentation. Do you mean we should
> > > > > > > > remove the empty release() callbacks?
> > > > > > >
> > > > > > > Yes, why are they needed?
> > > > > > >
> > > > > > > (hint, retorical question, you added them to remove the driver core
> > > > > > > warning when the device is removed, which means someone added them just
> > > > > > > because they thought that their code could ignore the hints that the
> > > > > > > driver core was telling them.)
> > > > > > >
> > > > > >
> > > > > > OK, I see.
> > > > > >
> > > > > > > Please properly free the memory here.
> > > > > > >
> > > > > >
> > > > > > One question is how to deal with the case if the device/kobject is
> > > > > > defined as a static variable. We should not need to free any resources
> > > > > > in this case. Or do you suggest just using dynamic allocation here?
> > > > >
> > > > > A kobject can NEVER be a static variable[1]. That's not how the driver
> > > > > model works at all. If this is how this code is written, it needs to be
> > > > > fixed.
> > > > >
> > > >
> > > > OK, I see.
> > > >
> > > > > [1] Ok, yes, drivers and busses and classes have static kobjects, ignore
> > > > > them...
> > > > >
> > > > > >
> > > > > > > > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > > > > > > > what you think it does. Please never ever ever do that. It
> > > > > > > > > too is a sign of a broken design.
> > > > > > > >
> > > > > > > > I don't find a good way to remove it. We have to make sure the module
> > > > > > > > can't be removed until all vduse devices are destroyed.
> > > > > > >
> > > > > > > That will happen automatically when the module is removed.
> > > > > > >
> > > > > > > > And I think __module_get(THIS_MODULE) should be safe in our case since
> > > > > > > > we always call it when we have a reference from open().
> > > > > > >
> > > > > > > What happened if someone removed the module _right before_ this was
> > > > > > > called? You can not grab your own reference count safely.
> > > > > > >
> > > > > >
> > > > > > I don't get you here. We should already grab a reference count from
> > > > > > open() before calling this. So it should fail if someone tries to
> > > > > > remove the module at this time.
> > > > >
> > > > > Then why are you trying to grab the module reference again?
> > > > >
> > > > > > > Please just remove it, it's not needed and is broken. There should not
> > > > > > > be any reason that the module can not be unloaded, UNLESS a file handle
> > > > > > > is open, and you properly handle that already.
> > > > > > >
> > > > > >
> > > > > > But in our case, I think we should prevent unloading the module If we
> > > > > > already created some vduse devices via /dev/vduse/control (note that
> > > > > > the control device's file handle could be closed after device
> > > > > > creation). Otherwise, we might get some crashes when accessing those
> > > > > > created vduse devices.
> > > > >
> > > > > Then the code is written incorrectly, this should not be an issue at
> > > > > all. Your devices will all be cleaned up properly before your code is
> > > > > unloaded from the system.
> > > > >
> > > >
> > > > In current design, the vduse device can't be cleaned up properly until
> > > > it is unbinded from the vDPA bus explicitly. So I use the extra
> > > > __module_get() to make sure we can't unload the module until the
> > > > device is cleaned up properly.
> > >
> > > Then something is wrong, it should not work that way.
> > >
> > > > > Note that no other driver or bus does this, what makes this different?
> > > > >
> > > >
> > > > I can see some similar behavior in loop and rbd modules.
> > >
> > > Never treat the loop code as a good example :)
> > >
> >
> > OK.
> >
> > > This should not be needed, when your module is unloaded, all devices it
> > > handled should be properly removed by it.
> > >
> >
> > I see. But it's not easy to achieve that currently. Maybe we need
> > something like DEVICE_NEEDS_RESET support in virtio core.
>
> This shouldn't be that hard, again, no other subsystem needs this. How
> about fix the other issues first and then we can work on that one.
>
Looks good to me.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:02 ` Yongji Xie
@ 2022-04-26 14:17 ` Michael S. Tsirkin
2022-04-26 14:17 ` Michael S. Tsirkin
1 sibling, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-04-26 14:17 UTC (permalink / raw)
To: Yongji Xie
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > This should not be needed, when your module is unloaded, all devices it
> > handled should be properly removed by it.
> >
>
> I see. But it's not easy to achieve that currently. Maybe we need
> something like DEVICE_NEEDS_RESET support in virtio core.
Not sure what the connection is.
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 14:17 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-04-26 14:17 UTC (permalink / raw)
To: Yongji Xie
Cc: Greg KH, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > This should not be needed, when your module is unloaded, all devices it
> > handled should be properly removed by it.
> >
>
> I see. But it's not easy to achieve that currently. Maybe we need
> something like DEVICE_NEEDS_RESET support in virtio core.
Not sure what the connection is.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:17 ` Michael S. Tsirkin
(?)
@ 2022-04-26 14:37 ` Yongji Xie
2022-04-26 14:38 ` Michael S. Tsirkin
-1 siblings, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 14:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > > This should not be needed, when your module is unloaded, all devices it
> > > handled should be properly removed by it.
> > >
> >
> > I see. But it's not easy to achieve that currently. Maybe we need
> > something like DEVICE_NEEDS_RESET support in virtio core.
>
> Not sure what the connection is.
>
If we want to force remove all working vduse devices during module
unload, we might need to send a DEVICE_NEEDS_RESET notification to
device driver to do some cleanup before, e.g., return error for all
inflight I/Os.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:37 ` Yongji Xie
@ 2022-04-26 14:38 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-04-26 14:38 UTC (permalink / raw)
To: Yongji Xie
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:37:17PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 10:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > > > This should not be needed, when your module is unloaded, all devices it
> > > > handled should be properly removed by it.
> > > >
> > >
> > > I see. But it's not easy to achieve that currently. Maybe we need
> > > something like DEVICE_NEEDS_RESET support in virtio core.
> >
> > Not sure what the connection is.
> >
>
> If we want to force remove all working vduse devices during module
> unload, we might need to send a DEVICE_NEEDS_RESET notification to
> device driver to do some cleanup before, e.g., return error for all
> inflight I/Os.
>
> Thanks,
> Yongji
IMHO DEVICE_NEEDS_RESET won't help much with that, it's more in
case device is still there but needs a reset to start working.
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-04-26 14:38 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-04-26 14:38 UTC (permalink / raw)
To: Yongji Xie
Cc: Greg KH, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Tue, Apr 26, 2022 at 10:37:17PM +0800, Yongji Xie wrote:
> On Tue, Apr 26, 2022 at 10:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > > > This should not be needed, when your module is unloaded, all devices it
> > > > handled should be properly removed by it.
> > > >
> > >
> > > I see. But it's not easy to achieve that currently. Maybe we need
> > > something like DEVICE_NEEDS_RESET support in virtio core.
> >
> > Not sure what the connection is.
> >
>
> If we want to force remove all working vduse devices during module
> unload, we might need to send a DEVICE_NEEDS_RESET notification to
> device driver to do some cleanup before, e.g., return error for all
> inflight I/Os.
>
> Thanks,
> Yongji
IMHO DEVICE_NEEDS_RESET won't help much with that, it's more in
case device is still there but needs a reset to start working.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 14:38 ` Michael S. Tsirkin
(?)
@ 2022-04-26 14:50 ` Yongji Xie
-1 siblings, 0 replies; 30+ messages in thread
From: Yongji Xie @ 2022-04-26 14:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Tue, Apr 26, 2022 at 10:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 26, 2022 at 10:37:17PM +0800, Yongji Xie wrote:
> > On Tue, Apr 26, 2022 at 10:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 10:02:02PM +0800, Yongji Xie wrote:
> > > > > This should not be needed, when your module is unloaded, all devices it
> > > > > handled should be properly removed by it.
> > > > >
> > > >
> > > > I see. But it's not easy to achieve that currently. Maybe we need
> > > > something like DEVICE_NEEDS_RESET support in virtio core.
> > >
> > > Not sure what the connection is.
> > >
> >
> > If we want to force remove all working vduse devices during module
> > unload, we might need to send a DEVICE_NEEDS_RESET notification to
> > device driver to do some cleanup before, e.g., return error for all
> > inflight I/Os.
> >
> > Thanks,
> > Yongji
>
> IMHO DEVICE_NEEDS_RESET won't help much with that, it's more in
> case device is still there but needs a reset to start working.
>
OK, we just want a reset without restarting work in our case. Looks
like we might need some other mechanism to achieve that.
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 7:36 [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access Xie Yongji
2022-04-26 8:07 ` Greg KH
@ 2022-06-02 4:55 ` Yongji Xie
2022-06-02 9:51 ` Michael S. Tsirkin
1 sibling, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-06-02 4:55 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: Luis Chamberlain, Andrew Morton, Greg KH, kernel test robot,
virtualization, stable
Ping.
On Tue, Apr 26, 2022 at 3:36 PM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> The control device has no drvdata. So we will get a
> NULL pointer dereference when accessing control
> device's msg_timeout attribute via sysfs:
>
> [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
>
> To fix it, don't create the unneeded attribute for
> control device anymore.
>
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f85d1a08ed87..160e40d03084 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>
> dev->minor = ret;
> dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> - dev->dev = device_create(vduse_class, NULL,
> - MKDEV(MAJOR(vduse_major), dev->minor),
> - dev, "%s", config->name);
> + dev->dev = device_create_with_groups(vduse_class, NULL,
> + MKDEV(MAJOR(vduse_major), dev->minor),
> + dev, vduse_dev_groups, "%s", config->name);
> if (IS_ERR(dev->dev)) {
> ret = PTR_ERR(dev->dev);
> goto err_dev;
> @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> return PTR_ERR(vduse_class);
>
> vduse_class->devnode = vduse_devnode;
> - vduse_class->dev_groups = vduse_dev_groups;
>
> ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> if (ret)
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-06-02 4:55 ` Yongji Xie
@ 2022-06-02 9:51 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-06-02 9:51 UTC (permalink / raw)
To: Yongji Xie
Cc: Jason Wang, Luis Chamberlain, Andrew Morton, Greg KH,
kernel test robot, virtualization, stable
On Thu, Jun 02, 2022 at 12:55:02PM +0800, Yongji Xie wrote:
> Ping.
Thanks for the reminder!
Will queue for rc2, rc1 has too much stuff already.
> On Tue, Apr 26, 2022 at 3:36 PM Xie Yongji <xieyongji@bytedance.com> wrote:
> >
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > - MKDEV(MAJOR(vduse_major), dev->minor),
> > - dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> > if (ret)
> > --
> > 2.20.1
> >
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-06-02 9:51 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-06-02 9:51 UTC (permalink / raw)
To: Yongji Xie
Cc: Greg KH, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
On Thu, Jun 02, 2022 at 12:55:02PM +0800, Yongji Xie wrote:
> Ping.
Thanks for the reminder!
Will queue for rc2, rc1 has too much stuff already.
> On Tue, Apr 26, 2022 at 3:36 PM Xie Yongji <xieyongji@bytedance.com> wrote:
> >
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > - MKDEV(MAJOR(vduse_major), dev->minor),
> > - dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");
> > if (ret)
> > --
> > 2.20.1
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-04-26 8:07 ` Greg KH
@ 2022-06-08 12:51 ` Michael S. Tsirkin
-1 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-06-08 12:51 UTC (permalink / raw)
To: Greg KH
Cc: Xie Yongji, jasowang, mcgrof, akpm, oliver.sang, virtualization, stable
On Tue, Apr 26, 2022 at 10:07:38AM +0200, Greg KH wrote:
> On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > - MKDEV(MAJOR(vduse_major), dev->minor),
> > - dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
>
> Ok, this looks much better.
>
> But wow, there are some problems in this code overall. I see a number
> of flat-out-wrong things in there that should have been caught by code
> reviews. Some examples:
> - empty release() callbacks. That is a huge sign the code
> design is wrong and broken and you are just trying to make the
> driver core quiet for some reason. The documentation in the
> kernel explains why this is not ok.
> - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> what you think it does. Please never ever ever do that. It
> too is a sign of a broken design.
> - no Documentation/ABI/ entries for the sysfs files here. I
> think it's burried in some other documentation file but that's
> not the correct place for it and if you run scripts/get_abi.pl
> with the code loaded it will rightly complain about this.
>
> Do you want to address these, or do you want patches for them?
>
> thanks,
>
> greg k-h
So, any patches?
--
MST
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-06-08 12:51 ` Michael S. Tsirkin
0 siblings, 0 replies; 30+ messages in thread
From: Michael S. Tsirkin @ 2022-06-08 12:51 UTC (permalink / raw)
To: Greg KH; +Cc: stable, virtualization, Xie Yongji, mcgrof, oliver.sang, akpm
On Tue, Apr 26, 2022 at 10:07:38AM +0200, Greg KH wrote:
> On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > The control device has no drvdata. So we will get a
> > NULL pointer dereference when accessing control
> > device's msg_timeout attribute via sysfs:
> >
> > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> >
> > To fix it, don't create the unneeded attribute for
> > control device anymore.
> >
> > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f85d1a08ed87..160e40d03084 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >
> > dev->minor = ret;
> > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > - dev->dev = device_create(vduse_class, NULL,
> > - MKDEV(MAJOR(vduse_major), dev->minor),
> > - dev, "%s", config->name);
> > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > + MKDEV(MAJOR(vduse_major), dev->minor),
> > + dev, vduse_dev_groups, "%s", config->name);
> > if (IS_ERR(dev->dev)) {
> > ret = PTR_ERR(dev->dev);
> > goto err_dev;
> > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > return PTR_ERR(vduse_class);
> >
> > vduse_class->devnode = vduse_devnode;
> > - vduse_class->dev_groups = vduse_dev_groups;
>
> Ok, this looks much better.
>
> But wow, there are some problems in this code overall. I see a number
> of flat-out-wrong things in there that should have been caught by code
> reviews. Some examples:
> - empty release() callbacks. That is a huge sign the code
> design is wrong and broken and you are just trying to make the
> driver core quiet for some reason. The documentation in the
> kernel explains why this is not ok.
> - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> what you think it does. Please never ever ever do that. It
> too is a sign of a broken design.
> - no Documentation/ABI/ entries for the sysfs files here. I
> think it's burried in some other documentation file but that's
> not the correct place for it and if you run scripts/get_abi.pl
> with the code loaded it will rightly complain about this.
>
> Do you want to address these, or do you want patches for them?
>
> thanks,
>
> greg k-h
So, any patches?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-06-08 12:51 ` Michael S. Tsirkin
(?)
@ 2022-06-08 13:25 ` Yongji Xie
2022-06-08 13:26 ` Parav Pandit via Virtualization
-1 siblings, 1 reply; 30+ messages in thread
From: Yongji Xie @ 2022-06-08 13:25 UTC (permalink / raw)
To: Michael S. Tsirkin, Parav Pandit
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
On Wed, Jun 8, 2022 at 8:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 26, 2022 at 10:07:38AM +0200, Greg KH wrote:
> > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > The control device has no drvdata. So we will get a
> > > NULL pointer dereference when accessing control
> > > device's msg_timeout attribute via sysfs:
> > >
> > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference, address: 00000000000000f8
> > > [ 132.850619][ T3644] RIP: 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094)
> > > [ 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59)
> > > [ 132.871164][ T3644] ? device_remove_bin_file (drivers/base/core.c:2088)
> > > [ 132.872082][ T3644] kernfs_seq_show (fs/kernfs/file.c:164)
> > > [ 132.872838][ T3644] seq_read_iter (fs/seq_file.c:230)
> > > [ 132.873578][ T3644] ? __vmalloc_area_node (mm/vmalloc.c:3041)
> > > [ 132.874532][ T3644] kernfs_fop_read_iter (fs/kernfs/file.c:238)
> > > [ 132.875513][ T3644] __kernel_read (fs/read_write.c:440 (discriminator 1))
> > > [ 132.876319][ T3644] kernel_read (fs/read_write.c:459)
> > > [ 132.877129][ T3644] kernel_read_file (fs/kernel_read_file.c:94)
> > > [ 132.877978][ T3644] kernel_read_file_from_fd (include/linux/file.h:45 fs/kernel_read_file.c:186)
> > > [ 132.879019][ T3644] __do_sys_finit_module (kernel/module.c:4207)
> > > [ 132.879930][ T3644] __ia32_sys_finit_module (kernel/module.c:4189)
> > > [ 132.880930][ T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112 arch/x86/entry/common.c:132)
> > > [ 132.881847][ T3644] entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > >
> > > To fix it, don't create the unneeded attribute for
> > > control device anymore.
> > >
> > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index f85d1a08ed87..160e40d03084 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> > >
> > > dev->minor = ret;
> > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > - dev->dev = device_create(vduse_class, NULL,
> > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > - dev, "%s", config->name);
> > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > + dev, vduse_dev_groups, "%s", config->name);
> > > if (IS_ERR(dev->dev)) {
> > > ret = PTR_ERR(dev->dev);
> > > goto err_dev;
> > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > return PTR_ERR(vduse_class);
> > >
> > > vduse_class->devnode = vduse_devnode;
> > > - vduse_class->dev_groups = vduse_dev_groups;
> >
> > Ok, this looks much better.
> >
> > But wow, there are some problems in this code overall. I see a number
> > of flat-out-wrong things in there that should have been caught by code
> > reviews. Some examples:
> > - empty release() callbacks. That is a huge sign the code
> > design is wrong and broken and you are just trying to make the
> > driver core quiet for some reason. The documentation in the
> > kernel explains why this is not ok.
> > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > what you think it does. Please never ever ever do that. It
> > too is a sign of a broken design.
> > - no Documentation/ABI/ entries for the sysfs files here. I
> > think it's burried in some other documentation file but that's
> > not the correct place for it and if you run scripts/get_abi.pl
> > with the code loaded it will rightly complain about this.
> >
> > Do you want to address these, or do you want patches for them?
> >
> > thanks,
> >
> > greg k-h
>
> So, any patches?
>
For empty release() callbacks, I think Parav is working on it based on
the discussion [1]. I can help test and send the patch if Parav wants.
For Documentation, I have sent a patch [2].
[1] https://www.spinics.net/lists/linux-virtualization/msg56518.html
[2] https://lore.kernel.org/all/CACGkMEuJeU6c1z8+_FqGtovbF+Sq8w_eQUcG8SHm_GXV5q7yNA@mail.gmail.com/
Thanks,
Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
2022-06-08 13:25 ` Yongji Xie
@ 2022-06-08 13:26 ` Parav Pandit via Virtualization
0 siblings, 0 replies; 30+ messages in thread
From: Parav Pandit @ 2022-06-08 13:26 UTC (permalink / raw)
To: Yongji Xie, Michael S. Tsirkin
Cc: Greg KH, Jason Wang, Luis Chamberlain, Andrew Morton,
kernel test robot, virtualization, stable
> From: Yongji Xie <xieyongji@bytedance.com>
> Sent: Wednesday, June 8, 2022 9:26 AM
>
> On Wed, Jun 8, 2022 at 8:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 10:07:38AM +0200, Greg KH wrote:
> > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > The control device has no drvdata. So we will get a NULL pointer
> > > > dereference when accessing control device's msg_timeout attribute
> > > > via sysfs:
> > > >
> > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference,
> > > > address: 00000000000000f8 [ 132.850619][ T3644] RIP:
> > > > 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094) [
> > > > 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59) [
> > > > 132.871164][ T3644] ? device_remove_bin_file
> > > > (drivers/base/core.c:2088) [ 132.872082][ T3644] kernfs_seq_show
> > > > (fs/kernfs/file.c:164) [ 132.872838][ T3644] seq_read_iter
> > > > (fs/seq_file.c:230) [ 132.873578][ T3644] ? __vmalloc_area_node
> > > > (mm/vmalloc.c:3041) [ 132.874532][ T3644] kernfs_fop_read_iter
> > > > (fs/kernfs/file.c:238) [ 132.875513][ T3644] __kernel_read
> > > > (fs/read_write.c:440 (discriminator 1)) [ 132.876319][ T3644]
> > > > kernel_read (fs/read_write.c:459) [ 132.877129][ T3644]
> > > > kernel_read_file (fs/kernel_read_file.c:94) [ 132.877978][ T3644]
> > > > kernel_read_file_from_fd (include/linux/file.h:45
> > > > fs/kernel_read_file.c:186) [ 132.879019][ T3644]
> > > > __do_sys_finit_module (kernel/module.c:4207) [ 132.879930][ T3644]
> > > > __ia32_sys_finit_module (kernel/module.c:4189) [ 132.880930][
> > > > T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112
> > > > arch/x86/entry/common.c:132) [ 132.881847][ T3644]
> > > > entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > >
> > > > To fix it, don't create the unneeded attribute for control device
> > > > anymore.
> > > >
> > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in
> > > > Userspace")
> > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > ---
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index f85d1a08ed87..160e40d03084 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct
> > > > vduse_dev_config *config,
> > > >
> > > > dev->minor = ret;
> > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > - dev->dev = device_create(vduse_class, NULL,
> > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > - dev, "%s", config->name);
> > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > + dev, vduse_dev_groups, "%s",
> > > > + config->name);
> > > > if (IS_ERR(dev->dev)) {
> > > > ret = PTR_ERR(dev->dev);
> > > > goto err_dev;
> > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > return PTR_ERR(vduse_class);
> > > >
> > > > vduse_class->devnode = vduse_devnode;
> > > > - vduse_class->dev_groups = vduse_dev_groups;
> > >
> > > Ok, this looks much better.
> > >
> > > But wow, there are some problems in this code overall. I see a
> > > number of flat-out-wrong things in there that should have been
> > > caught by code reviews. Some examples:
> > > - empty release() callbacks. That is a huge sign the code
> > > design is wrong and broken and you are just trying to make the
> > > driver core quiet for some reason. The documentation in the
> > > kernel explains why this is not ok.
> > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > what you think it does. Please never ever ever do that. It
> > > too is a sign of a broken design.
> > > - no Documentation/ABI/ entries for the sysfs files here. I
> > > think it's burried in some other documentation file but that's
> > > not the correct place for it and if you run scripts/get_abi.pl
> > > with the code loaded it will rightly complain about this.
> > >
> > > Do you want to address these, or do you want patches for them?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > So, any patches?
> >
>
> For empty release() callbacks, I think Parav is working on it based on the
> discussion [1]. I can help test and send the patch if Parav wants.
>
As there are no more comments, I will send the patch for vduse this week based on [1] which was previously in email form.
> For Documentation, I have sent a patch [2].
>
> [1] https://www.spinics.net/lists/linux-virtualization/msg56518.html
> [2]
> https://lore.kernel.org/all/CACGkMEuJeU6c1z8+_FqGtovbF+Sq8w_eQUcG8
> SHm_GXV5q7yNA@mail.gmail.com/
>
> Thanks,
> Yongji
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access
@ 2022-06-08 13:26 ` Parav Pandit via Virtualization
0 siblings, 0 replies; 30+ messages in thread
From: Parav Pandit via Virtualization @ 2022-06-08 13:26 UTC (permalink / raw)
To: Yongji Xie, Michael S. Tsirkin
Cc: Greg KH, stable, virtualization, Luis Chamberlain,
kernel test robot, Andrew Morton
> From: Yongji Xie <xieyongji@bytedance.com>
> Sent: Wednesday, June 8, 2022 9:26 AM
>
> On Wed, Jun 8, 2022 at 8:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 10:07:38AM +0200, Greg KH wrote:
> > > On Tue, Apr 26, 2022 at 03:36:56PM +0800, Xie Yongji wrote:
> > > > The control device has no drvdata. So we will get a NULL pointer
> > > > dereference when accessing control device's msg_timeout attribute
> > > > via sysfs:
> > > >
> > > > [ 132.841881][ T3644] BUG: kernel NULL pointer dereference,
> > > > address: 00000000000000f8 [ 132.850619][ T3644] RIP:
> > > > 0010:msg_timeout_show (drivers/vdpa/vdpa_user/vduse_dev.c:1271)
> > > > [ 132.869447][ T3644] dev_attr_show (drivers/base/core.c:2094) [
> > > > 132.870215][ T3644] sysfs_kf_seq_show (fs/sysfs/file.c:59) [
> > > > 132.871164][ T3644] ? device_remove_bin_file
> > > > (drivers/base/core.c:2088) [ 132.872082][ T3644] kernfs_seq_show
> > > > (fs/kernfs/file.c:164) [ 132.872838][ T3644] seq_read_iter
> > > > (fs/seq_file.c:230) [ 132.873578][ T3644] ? __vmalloc_area_node
> > > > (mm/vmalloc.c:3041) [ 132.874532][ T3644] kernfs_fop_read_iter
> > > > (fs/kernfs/file.c:238) [ 132.875513][ T3644] __kernel_read
> > > > (fs/read_write.c:440 (discriminator 1)) [ 132.876319][ T3644]
> > > > kernel_read (fs/read_write.c:459) [ 132.877129][ T3644]
> > > > kernel_read_file (fs/kernel_read_file.c:94) [ 132.877978][ T3644]
> > > > kernel_read_file_from_fd (include/linux/file.h:45
> > > > fs/kernel_read_file.c:186) [ 132.879019][ T3644]
> > > > __do_sys_finit_module (kernel/module.c:4207) [ 132.879930][ T3644]
> > > > __ia32_sys_finit_module (kernel/module.c:4189) [ 132.880930][
> > > > T3644] do_int80_syscall_32 (arch/x86/entry/common.c:112
> > > > arch/x86/entry/common.c:132) [ 132.881847][ T3644]
> > > > entry_INT80_compat (arch/x86/entry/entry_64_compat.S:419)
> > > >
> > > > To fix it, don't create the unneeded attribute for control device
> > > > anymore.
> > > >
> > > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in
> > > > Userspace")
> > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > ---
> > > > drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++----
> > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index f85d1a08ed87..160e40d03084 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -1344,9 +1344,9 @@ static int vduse_create_dev(struct
> > > > vduse_dev_config *config,
> > > >
> > > > dev->minor = ret;
> > > > dev->msg_timeout = VDUSE_MSG_DEFAULT_TIMEOUT;
> > > > - dev->dev = device_create(vduse_class, NULL,
> > > > - MKDEV(MAJOR(vduse_major), dev->minor),
> > > > - dev, "%s", config->name);
> > > > + dev->dev = device_create_with_groups(vduse_class, NULL,
> > > > + MKDEV(MAJOR(vduse_major), dev->minor),
> > > > + dev, vduse_dev_groups, "%s",
> > > > + config->name);
> > > > if (IS_ERR(dev->dev)) {
> > > > ret = PTR_ERR(dev->dev);
> > > > goto err_dev;
> > > > @@ -1595,7 +1595,6 @@ static int vduse_init(void)
> > > > return PTR_ERR(vduse_class);
> > > >
> > > > vduse_class->devnode = vduse_devnode;
> > > > - vduse_class->dev_groups = vduse_dev_groups;
> > >
> > > Ok, this looks much better.
> > >
> > > But wow, there are some problems in this code overall. I see a
> > > number of flat-out-wrong things in there that should have been
> > > caught by code reviews. Some examples:
> > > - empty release() callbacks. That is a huge sign the code
> > > design is wrong and broken and you are just trying to make the
> > > driver core quiet for some reason. The documentation in the
> > > kernel explains why this is not ok.
> > > - __module_get(THIS_MODULE); That's racy, buggy, and doesn't do
> > > what you think it does. Please never ever ever do that. It
> > > too is a sign of a broken design.
> > > - no Documentation/ABI/ entries for the sysfs files here. I
> > > think it's burried in some other documentation file but that's
> > > not the correct place for it and if you run scripts/get_abi.pl
> > > with the code loaded it will rightly complain about this.
> > >
> > > Do you want to address these, or do you want patches for them?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > So, any patches?
> >
>
> For empty release() callbacks, I think Parav is working on it based on the
> discussion [1]. I can help test and send the patch if Parav wants.
>
As there are no more comments, I will send the patch for vduse this week based on [1] which was previously in email form.
> For Documentation, I have sent a patch [2].
>
> [1] https://www.spinics.net/lists/linux-virtualization/msg56518.html
> [2]
> https://lore.kernel.org/all/CACGkMEuJeU6c1z8+_FqGtovbF+Sq8w_eQUcG8
> SHm_GXV5q7yNA@mail.gmail.com/
>
> Thanks,
> Yongji
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-06-08 13:26 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-26 7:36 [PATCH v2] vduse: Fix NULL pointer dereference on sysfs access Xie Yongji
2022-04-26 8:07 ` Greg KH
2022-04-26 8:07 ` Greg KH
2022-04-26 9:41 ` Yongji Xie
2022-04-26 10:26 ` Greg KH
2022-04-26 10:26 ` Greg KH
2022-04-26 12:30 ` Yongji Xie
2022-04-26 12:44 ` Greg KH
2022-04-26 12:44 ` Greg KH
2022-04-26 13:17 ` Yongji Xie
2022-04-26 13:34 ` Greg KH
2022-04-26 13:34 ` Greg KH
2022-04-26 14:02 ` Yongji Xie
2022-04-26 14:09 ` Greg KH
2022-04-26 14:09 ` Greg KH
2022-04-26 14:15 ` Yongji Xie
2022-04-26 14:17 ` Michael S. Tsirkin
2022-04-26 14:17 ` Michael S. Tsirkin
2022-04-26 14:37 ` Yongji Xie
2022-04-26 14:38 ` Michael S. Tsirkin
2022-04-26 14:38 ` Michael S. Tsirkin
2022-04-26 14:50 ` Yongji Xie
2022-06-08 12:51 ` Michael S. Tsirkin
2022-06-08 12:51 ` Michael S. Tsirkin
2022-06-08 13:25 ` Yongji Xie
2022-06-08 13:26 ` Parav Pandit
2022-06-08 13:26 ` Parav Pandit via Virtualization
2022-06-02 4:55 ` Yongji Xie
2022-06-02 9:51 ` Michael S. Tsirkin
2022-06-02 9:51 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.