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

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

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.