| Message ID | 20240115063434.20278-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] vfio: Use WARN_ON for low-probability allocation failure issue in vfio_pci_bus_notifier | expand |
On Monday, January 15, 2024 2:35 PM, Kunwu Chan wrote: > kasprintf() returns a pointer to dynamically allocated memory which can be > NULL upon failure. > > This is a blocking notifier callback, so errno isn't a proper return value. Use > WARN_ON to small allocation failures. > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > v2: Use WARN_ON instead of return errno > --- > drivers/vfio/pci/vfio_pci_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 1cbc990d42e0..61aa19666050 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -2047,6 +2047,7 @@ static int vfio_pci_bus_notifier(struct notifier_block > *nb, > pci_name(pdev)); > pdev->driver_override = kasprintf(GFP_KERNEL, "%s", > vdev->vdev.ops->name); > + WARN_ON(!pdev->driver_override); Saw Alex's comments on v1. Curious why not return "NOTIFY_BAD" on errors though less likely? Similar examples could be found in kvm_pm_notifier_call, kasan_mem_notifier etc.
On Mon, 15 Jan 2024 15:41:02 +0000 "Wang, Wei W" <wei.w.wang@intel.com> wrote: > On Monday, January 15, 2024 2:35 PM, Kunwu Chan wrote: > > kasprintf() returns a pointer to dynamically allocated memory which can be > > NULL upon failure. > > > > This is a blocking notifier callback, so errno isn't a proper return value. Use > > WARN_ON to small allocation failures. > > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > --- > > v2: Use WARN_ON instead of return errno > > --- > > drivers/vfio/pci/vfio_pci_core.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > > index 1cbc990d42e0..61aa19666050 100644 > > --- a/drivers/vfio/pci/vfio_pci_core.c > > +++ b/drivers/vfio/pci/vfio_pci_core.c > > @@ -2047,6 +2047,7 @@ static int vfio_pci_bus_notifier(struct notifier_block > > *nb, > > pci_name(pdev)); > > pdev->driver_override = kasprintf(GFP_KERNEL, "%s", > > vdev->vdev.ops->name); > > + WARN_ON(!pdev->driver_override); > > Saw Alex's comments on v1. Curious why not return "NOTIFY_BAD" on errors though > less likely? Similar examples could be found in kvm_pm_notifier_call, kasan_mem_notifier etc. If the statement is that there are notifier call chains that return NOTIFY_BAD, I would absolutely agree, but the return value needs to be examined from the context of the caller. BUS_NOTIFY_ADD_DEVICE is notified via bus_notify() in device_add(). What does it accomplish to return NOTIFY_BAD in a chain that ignores the return value? At best we're preventing callbacks further down the chain from being called. That doesn't seem obviously beneficial either. The scenario here is similar to that in fail_iommu_bus_notify() where they've chosen to trigger a pr_warn() if they're unable to crease sysfs entries. In fact, a pci_warn(), maybe even pci_err() might be a better alternative here than a WARN_ON(). Thanks, Alex
On Tuesday, January 16, 2024 12:29 AM, Alex Williamson wrote: > > On Monday, January 15, 2024 2:35 PM, Kunwu Chan wrote: > > > kasprintf() returns a pointer to dynamically allocated memory which > > > can be NULL upon failure. > > > > > > This is a blocking notifier callback, so errno isn't a proper return > > > value. Use WARN_ON to small allocation failures. > > > > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > > --- > > > v2: Use WARN_ON instead of return errno > > > --- > > > drivers/vfio/pci/vfio_pci_core.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_core.c > > > b/drivers/vfio/pci/vfio_pci_core.c > > > index 1cbc990d42e0..61aa19666050 100644 > > > --- a/drivers/vfio/pci/vfio_pci_core.c > > > +++ b/drivers/vfio/pci/vfio_pci_core.c > > > @@ -2047,6 +2047,7 @@ static int vfio_pci_bus_notifier(struct > > > notifier_block *nb, > > > pci_name(pdev)); > > > pdev->driver_override = kasprintf(GFP_KERNEL, "%s", > > > vdev->vdev.ops->name); > > > + WARN_ON(!pdev->driver_override); > > > > Saw Alex's comments on v1. Curious why not return "NOTIFY_BAD" on > > errors though less likely? Similar examples could be found in > kvm_pm_notifier_call, kasan_mem_notifier etc. > > If the statement is that there are notifier call chains that return NOTIFY_BAD, I > would absolutely agree, but the return value needs to be examined from the > context of the caller. BUS_NOTIFY_ADD_DEVICE is notified via bus_notify() in > device_add(). What does it accomplish to return NOTIFY_BAD in a chain that > ignores the return value? At best we're preventing callbacks further down the > chain from being called. > That doesn't seem obviously beneficial either. OK, thanks for the clarification. My curiosity came from the statement "This is a blocking notifier callback, so errno isn't a proper return value". Probably the commit log needs some rewording.
On Mon, 15 Jan 2024 14:34:34 +0800 Kunwu Chan <chentao@kylinos.cn> wrote: > kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. > > This is a blocking notifier callback, so errno isn't a proper return > value. Use WARN_ON to small allocation failures. > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > v2: Use WARN_ON instead of return errno > --- > drivers/vfio/pci/vfio_pci_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index 1cbc990d42e0..61aa19666050 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -2047,6 +2047,7 @@ static int vfio_pci_bus_notifier(struct notifier_block *nb, > pci_name(pdev)); > pdev->driver_override = kasprintf(GFP_KERNEL, "%s", > vdev->vdev.ops->name); > + WARN_ON(!pdev->driver_override); > } else if (action == BUS_NOTIFY_BOUND_DRIVER && > pdev->is_virtfn && physfn == vdev->pdev) { > struct pci_driver *drv = pci_dev_driver(pdev); Applied to vfio next branch for v6.9. Thanks, Alex
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 1cbc990d42e0..61aa19666050 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -2047,6 +2047,7 @@ static int vfio_pci_bus_notifier(struct notifier_block *nb, pci_name(pdev)); pdev->driver_override = kasprintf(GFP_KERNEL, "%s", vdev->vdev.ops->name); + WARN_ON(!pdev->driver_override); } else if (action == BUS_NOTIFY_BOUND_DRIVER && pdev->is_virtfn && physfn == vdev->pdev) { struct pci_driver *drv = pci_dev_driver(pdev);
kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. This is a blocking notifier callback, so errno isn't a proper return value. Use WARN_ON to small allocation failures. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- v2: Use WARN_ON instead of return errno --- drivers/vfio/pci/vfio_pci_core.c | 1 + 1 file changed, 1 insertion(+)