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

[Paraschiv, Andra-Irina]

On 24/05/2020 09:32, Greg KH wrote:
> 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.

I updated the static calls checks and removed the WARN_ONs. And 
ratelimited is used now only in the ioctl call paths.

Thank you both.

Andra



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.