netdev - Re: [PATCH] [v7] net: emac: emac gigabit ethernet controller 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]

Date:	Tue, 9 Aug 2016 20:09:36 -0500
From:	Timur Tabi <timur@...eaurora.org>
To:	Florian Fainelli <f.fainelli@...il.com>,
	Lino Sanfilippo <LinoSanfilippo@....de>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, sdharia@...eaurora.org,
	shankerd@...eaurora.org, vikrams@...eaurora.org,
	cov@...eaurora.org, gavidov@...eaurora.org, robh+dt@...nel.org,
	andrew@...n.ch, bjorn.andersson@...aro.org, mlangsdo@...hat.com,
	jcm@...hat.com, agross@...eaurora.org, davem@...emloft.net
Subject: Re: [PATCH] [v7] net: emac: emac gigabit ethernet controller driver

Florian Fainelli wrote:

> nr_frags can't be bigger than MAX_SKB_FRAGS, hence these checks all
> other drivers do against 1 + MAX_SKB_FRAGS.

Doh, I just realized something.  emac_mac_tx_buf_send() just needs to 
make sure that there's enough room for ONE skb.  For some reason I 
thought it had to make sure there's enough room for multiple SKBs.

Now it makes a lot more sense.  Thank you.

So it looks like a given SKB can occupy 3 + nr_frags descriptors.  So I 
need to change that line to:

	if (emac_tpd_num_free_descs(tx_q) < (MAX_SKB_FRAGS + 3))
		netif_stop_queue(adpt->netdev);

Question, some  drivers do <= instead of just <, like this:

	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
		netif_tx_stop_queue(txq);

Is it necessary to stop the queue if there exactly enough descriptors to 
hold an SKB?  Shouldn't the above be this instead:

	if (ring->free_bds < (MAX_SKB_FRAGS + 1))
		netif_tx_stop_queue(txq);


>> However, I'm confused about one thing.  Almost every other driver just
>> sets "netdev->mtu = new_mtu" and does nothing else.  I can't find any
>> other driver that actually stops the RX path, reprograms the hardware,
>> and then restarts the RX path.  I know this is a stupid question, but
>> why is my driver doing that?
>
> Most drivers allocate RX buffer sizes that are usually bigger than the
> MTU, but would probably silently fail or expose transient behavior once
> the MTU changes to greater than the size pre-defined.

So it looks like the real problem is a race condition between

	adpt->rxbuf_size = new_mtu > EMAC_DEF_RX_BUF_SIZE ?
		ALIGN(max_frame, 8) : EMAC_DEF_RX_BUF_SIZE;

and

	if (netif_running(netdev))
		return emac_reinit_locked(adpt);


That is, if the interface is running, I set rxbuf_size.  If suddenly I 
receive some packets, then the driver will use the wrong buffer size.

Is there an easy way for me to stop the RX path before I set rxbuf_size? 
  Some netif_xxx function I can call?


-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.