linux-kernel - Re: [PATCH v2 01/15] NTB: Rename NTB messaging API methods

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Dec 2017 12:11:48 -0700
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Serge Semin <fancer.lancer@...il.com>, Jon Mason <jdmason@...zu.us>
Cc:     Dave Jiang <dave.jiang@...el.com>,
        "Hubbe, Allen" <Allen.Hubbe@....com>,
        "S-k, Shyam-sundar" <Shyam-sundar.S-k@....com>,
        "Yu, Xiangliang" <Xiangliang.Yu@....com>,
        Gary R Hook <gary.hook@....com>, Sergey.Semin@...latforms.ru,
        linux-ntb <linux-ntb@...glegroups.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 01/15] NTB: Rename NTB messaging API methods



On 05/12/17 10:31 AM, Serge Semin wrote:
>>> -static int idt_ntb_msg_read(struct ntb_dev *ntb, int midx, int *pidx, u32 *msg)
>>> +static u32 idt_ntb_msg_read(struct ntb_dev *ntb, int *pidx, int midx)
>>>   {
>>>          struct idt_ntb_dev *ndev = to_ndev_ntb(ntb);
>>>
>>>          if (midx < 0 || IDT_MSG_CNT <= midx)
>>> -               return -EINVAL;
>>> +               return (u32)-1;
>>
>> Please don't do this.  If this is an error return standard error
>> number.  And why are we casting to an unsigned int now?
>>
> 
> We discussed these changes on the v1 series. Additionally I asked similar
> question sometime ago even before the patchset was introduced.
> This patch is made to provide the Message interface similar to the Scratchpad
> one. I didn't introduce anything new or unjustified. As you can see the
> spad_read/peer_spad_read methods return u32 type too. As well as the
> Intel/AMD callbacks. The functions are intentionally made to return FFs
> in case if some of the passed arguments get out from the allowed limits.
> In such circumstances the return value emulates a situation like if user would
> reference an invalid PCIe MMIO address. Since the 32-bits register can in general
> have any value including -errno ones, then returning an error within the NTB API
> would be incorrect. I remember Allen described it this way.
> Nobody argued about it last time. If you think it's incorrect, then it should be
> changed in both Scratchpad and Message register interfaces.

I agree with Jon to not make this change. The original interface is 
better. Making the interface similar to spad_read() has no value 
especially seeing it makes it less correct. As you mention, *msg can be 
any value (even -1) so this restricts the values possible message values 
(making future potential hidden bugs) and removes error information 
(making debugging more difficult).

I haven't really looked into it but, if anything, it would make sense to 
make the spad_read function more like this one.

Logan