diff mbox series

[v2] vfio: Use WARN_ON for low-probability allocation failure issue in vfio_pci_bus_notifier

Message ID 20240115063434.20278-1-chentao@kylinos.cn
State New
Headers
Series [v2] vfio: Use WARN_ON for low-probability allocation failure issue in vfio_pci_bus_notifier |

Commit Message

Kunwu Chan Jan. 15, 2024, 6:34 a.m. UTC 
  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(+)

Comments

Wang, Wei W Jan. 15, 2024, 3:41 p.m. UTC | #1 
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.
Alex Williamson Jan. 15, 2024, 4:28 p.m. UTC | #2 
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
Wang, Wei W Jan. 16, 2024, 12:08 p.m. UTC | #3 
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.
Alex Williamson Feb. 22, 2024, 9:37 p.m. UTC | #4 
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 mbox series

Patch

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);