netdev - Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

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]

Message-ID: <dleftjczza7xgf.fsf%l.stelmach@samsung.com>
Date:   Wed, 16 Dec 2020 13:21:52 +0100
From:   Lukasz Stelmach <l.stelmach@...sung.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>, jim.cromie@...il.com,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v8 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet
 Adapter Driver

It was <2020-12-15 wto 17:46>, when Jakub Kicinski wrote:
> On Wed, 16 Dec 2020 01:42:03 +0100 Lukasz Stelmach wrote:
>>>> +	ax_local->stats.rx_packets++;
>>>> +	ax_local->stats.rx_bytes += skb->len;
>>>> +	skb->dev = ndev;
>>>> +
>>>> +	skb->truesize = skb->len + sizeof(struct sk_buff);  
>>>
>>> Why do you modify truesize?
>>>  
>> 
>> I don't know. Although uncommon, this appears in a few usb drivers, so I
>> didn't think much about it when I ported this code.
>
> I'd guess they do aggregation. I wouldn't touch it in your driver.
>

Removed.

>>>> +	u8			plat_endian;
>>>> +		#define PLAT_LITTLE_ENDIAN	0
>>>> +		#define PLAT_BIG_ENDIAN		1  
>>>
>>> Why do you store this little nugget of information?
>>>  
>> 
>> I don't know*. The hardware enables endianness detection by providing a
>> constant value (0x1234) in one of its registers. Unfortunately I don't
>> have a big-endian board with this chip to check if it is necessary to
>> alter AX_READ/AX_WRITE in any way.
>
> Yeah, may be hard to tell what magic the device is doing.
> I was mostly saying that you don't seem to use this information,
> so the member of the struct can be removed IIRC.
>

Removed.

>>> These all look like multiple of 2 bytes. Why do they need to be packed?
>> 
>> These are structures sent to and returned from the hardware. They are
>> prepended and appended to the network packets. I think it is good to
>> keep them packed, so compilers won't try any tricks.
>
> Compilers can't play tricks on memory layout of structures, the
> standard is pretty strict about that. Otherwise ABIs would never work.
> We prefer not to unnecessarily pack structures in the neworking code,
> because it generates byte by byte loads on architectures which can't 
> do unaligned accesses.

Indeed, a struct of three u16 is 6 bytes long. I was worried it may be
rounded up to 8. Removed.

>>> No need to return some specific pattern on failure? Like 0xffff?
>> 
>> All registers are 16 bit wide. I am afraid it isn't safe to assume that
>> there is a 16 bit value we could use. Chances that SPI goes south are
>> pretty slim. And if it does, there isn't much more than reporting an
>> error we can do about it anyway.
>> 
>> One thing I can think of is to change axspi_* to (s32), return -1,
>> somehow (how?) shutdown the device in AX_*.
>
> I'm mostly concerned about potentially random data left over in the
> buffer. Seems like it could lead to hard to repro bugs. Hence the
> suggestion to return a constant of your choosing on error, doesn't
> really matter what as long as it's a known constant.

I see, that makes sense, indeed.

So, the only thing that's left is pskb_expand_head(). I need to wrap my
head around it. Can you tell me how a cloned skb is different and why
there may be separate branch for it?

Thank you.
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Download attachment "signature.asc" of type "application/pgp-signature" (488 bytes)