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

[Alex Williamson]

On Mon, 15 Jan 2024 15:41:02 +0000
"Wang, Wei W" <wei.w.wang@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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