netdev - Re: [PATCHv2] 3c59x: fix missing dma_mapping_error check and bad ring refill logic

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: <20180103151333.GB22129@hmswarspite.think-freely.org>
Date:   Wed, 3 Jan 2018 10:13:33 -0500
From:   Neil Horman <nhorman@...hat.com>
To:     David Miller <davem@...emloft.net>
Cc:     nhorman@...driver.com, netdev@...r.kernel.org,
        tedheadster@...il.com, klassert@...hematik.tu-chemnitz.de
Subject: Re: [PATCHv2] 3c59x: fix missing dma_mapping_error check and bad
 ring refill logic

On Wed, Jan 03, 2018 at 09:58:49AM -0500, David Miller wrote:
> From: Neil Horman <nhorman@...driver.com>
> Date: Wed,  3 Jan 2018 09:44:15 -0500
> 
> > A few spots in 3c59x missed calls to dma_mapping_error checks, casuing
> > WARN_ONS to trigger.  Clean those up.  While we're at it, refactor the
> > refill code a bit so that if skb allocation or dma mapping fails, we
> > recycle the existing buffer.  This prevents holes in the rx ring, and
> > makes for much simpler logic
> > 
> > Note: This is compile only tested.  Ted, if you could run this and
> > confirm that it continues to work properly, I would appreciate it, as I
> > currently don't have access to this hardware
> > 
> > Signed-off-by: Neil Horman <nhorman@...hat.com>
> > CC: Steffen Klassert <klassert@...hematik.tu-chemnitz.de>
> > CC: "David S. Miller" <davem@...emloft.net>
> > Reported-by: tedheadster@...il.com
> 
> See my other reply.
> 
> Your RX handling must become more sophisticated.
> 
Yes, I understood your previous reply.

> This is exactly what we tell driver authors to do.  If you cannot allocate
> or DMA map a replacement RX buffer, you _MUST_ recycle the existing buffer
> back to the chip rather than pass it up to the stack.
> 
Thats exactly what this patch does, instead of creating a second loop to
traverse all the emptied ring buffers, now I:

1) Pre-allocate a new skb when I know I'm going to receive the in-place skb
2) Map the skb into the appropriate dma device domain
3) If (1) and (2) succede, then I swap the newly allocate skb and dma address
with the old one and recieve the old into the network stack
4) If (1) or (2) fail, then I goto clear_complete, which leaves the old skb and
dma address in place, sets the buffer status back to 0 (indicating completion),
and write the new ring status back to the hardware

This is what you wanted, a pre-allocate and swap-if-successful, recycle-if-not
approach, rather than the leave-a-hole-in-the-ring approach that is there
currently, no?  Or did I miss something else?

Neil