Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver

[Greg KH <greg@kroah.com>]

On Sat, May 23, 2020 at 10:25:25PM +0200, Alexander Graf wrote:
> Hey Greg,
> 
> On 22.05.20 09:04, Greg KH wrote:
> > 
> > On Fri, May 22, 2020 at 09:29:32AM +0300, Andra Paraschiv wrote:
> > > +/**
> > > + * ne_setup_msix - Setup MSI-X vectors for the PCI device.
> > > + *
> > > + * @pdev: PCI device to setup the MSI-X for.
> > > + *
> > > + * @returns: 0 on success, negative return value on failure.
> > > + */
> > > +static int ne_setup_msix(struct pci_dev *pdev)
> > > +{
> > > +     struct ne_pci_dev *ne_pci_dev = NULL;
> > > +     int nr_vecs = 0;
> > > +     int rc = -EINVAL;
> > > +
> > > +     if (WARN_ON(!pdev))
> > > +             return -EINVAL;
> > 
> > How can this ever happen?  If it can not, don't test for it.  If it can,
> > don't warn for it as that will crash systems that do panic-on-warn, just
> > test and return an error.
> 
> I think the point here is to catch situations that should never happen, but
> keep a sanity check in in case they do happen. This would've usually been a
> BUG_ON, but people tend to dislike those these days because they can bring
> down your system ...

Same for WARN_ON when you run with panic-on-warn enabled :(

> So in this particular case here I agree that it's a bit silly to check
> whether pdev is != NULL. In other device code internal APIs though it's not
> quite as clear of a cut. I by far prefer code that tells me it's broken over
> reverse engineering stray pointer accesses ...

For static calls where you control the callers, don't do checks like
this.  Otherwise the kernel would just be full of these all over the
place and things would slow down.  It's just not needed.

> > > +     ne_pci_dev = pci_get_drvdata(pdev);
> > > +     if (WARN_ON(!ne_pci_dev))
> > > +             return -EINVAL;
> > 
> > Same here, don't use WARN_ON if at all possible.
> > 
> > > +
> > > +     nr_vecs = pci_msix_vec_count(pdev);
> > > +     if (nr_vecs < 0) {
> > > +             rc = nr_vecs;
> > > +
> > > +             dev_err_ratelimited(&pdev->dev,
> > > +                                 NE "Error in getting vec count [rc=%d]\n",
> > > +                                 rc);
> > > +
> > 
> > Why ratelimited, can this happen over and over and over?
> 
> In this particular function, no, so here it really should just be dev_err.
> Other functions are implicitly callable from user space through an ioctl,
> which means they really need to stay rate limited.

Think through these as the driver seems to ONLY use these ratelimited
calls right now, which is not correct.

Also, if a user can create a printk, that almost always is not a good
idea.  But yes, those should be ratelimited.

thanks,

greg k-h