linux-kernel - Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

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:   Wed, 23 Jun 2021 19:31:00 +0200
From:   Greg KH <greg@...ah.com>
To:     Rocco Yue <rocco.yue@...iatek.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>,
        Sean Wang <sean.wang@...iatek.com>,
        Mark Lee <Mark-MC.Lee@...iatek.com>, netdev@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, bpf@...r.kernel.org,
        wsd_upstream@...iatek.com, chao.song@...iatek.com,
        zhuoliang.zhang@...iatek.com, kuohong.wang@...iatek.com
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of
 ccmni

On Wed, Jun 23, 2021 at 07:34:52PM +0800, Rocco Yue wrote:
> +static int ccmni_open(struct net_device *ccmni_dev)
> +{
> +	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
> +
> +	netif_tx_start_all_queues(ccmni_dev);
> +	netif_carrier_on(ccmni_dev);
> +
> +	if (atomic_inc_return(&ccmni->usage) > 1) {
> +		atomic_dec(&ccmni->usage);
> +		netdev_err(ccmni_dev, "dev already open\n");
> +		return -EINVAL;

You only check this _AFTER_ starting up?  If so, why even check a count
at all?  Why does it matter as it's not keeping anything from working
here.



> +	}
> +
> +	return 0;
> +}
> +
> +static int ccmni_close(struct net_device *ccmni_dev)
> +{
> +	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
> +
> +	atomic_dec(&ccmni->usage);
> +	netif_tx_disable(ccmni_dev);
> +
> +	return 0;
> +}
> +
> +static netdev_tx_t
> +ccmni_start_xmit(struct sk_buff *skb, struct net_device *ccmni_dev)
> +{
> +	struct ccmni_inst *ccmni = NULL;
> +
> +	if (unlikely(!ccmni_hook_ready))
> +		goto tx_ok;
> +
> +	if (!skb || !ccmni_dev)
> +		goto tx_ok;
> +
> +	ccmni = netdev_priv(ccmni_dev);
> +
> +	/* some process can modify ccmni_dev->mtu */
> +	if (skb->len > ccmni_dev->mtu) {
> +		netdev_err(ccmni_dev, "xmit fail: len(0x%x) > MTU(0x%x, 0x%x)",
> +			   skb->len, CCMNI_MTU, ccmni_dev->mtu);
> +		goto tx_ok;
> +	}
> +
> +	/* hardware driver send packet will return a negative value
> +	 * ask the Linux netdevice to stop the tx queue
> +	 */
> +	if ((s_ccmni_ctlb->xmit_pkt(ccmni->index, skb, 0)) < 0)
> +		return NETDEV_TX_BUSY;
> +
> +	return NETDEV_TX_OK;
> +tx_ok:
> +	dev_kfree_skb(skb);
> +	ccmni_dev->stats.tx_dropped++;
> +	return NETDEV_TX_OK;
> +}
> +
> +static int ccmni_change_mtu(struct net_device *ccmni_dev, int new_mtu)
> +{
> +	if (new_mtu < 0 || new_mtu > CCMNI_MTU)
> +		return -EINVAL;
> +
> +	if (unlikely(!ccmni_dev))
> +		return -EINVAL;
> +
> +	ccmni_dev->mtu = new_mtu;
> +	return 0;
> +}
> +
> +static void ccmni_tx_timeout(struct net_device *ccmni_dev, unsigned int txqueue)
> +{
> +	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
> +
> +	ccmni_dev->stats.tx_errors++;
> +	if (atomic_read(&ccmni->usage) > 0)
> +		netif_tx_wake_all_queues(ccmni_dev);

Why does it matter what the reference count is?  What happens if it
drops _RIGHT_ after testing for it?

Anytime you do an atomic_read() call, it's almost always a sign that the
logic is not correct.

Again, why have this reference count at all?  What is it protecting?

> +/* exposed API
> + * receive incoming datagrams from the Modem and push them to the
> + * kernel networking system
> + */
> +int ccmni_rx_push(unsigned int ccmni_idx, struct sk_buff *skb)

Ah, so this driver doesn't really do anything on its own, as there is no
modem driver for it.

So without a modem driver, it will never be used?  Please submit the
modem driver at the same time, otherwise it's impossible to review this
correctly.

thanks,

greg k-h