linux-kernel - Re: [PATCH net-next 2/2] appletalk: tashtalk: Add LocalTalk line discipline driver for AppleTalk using a TashTalk adapter

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: <20240819094709.GD11472@kernel.org>
Date: Mon, 19 Aug 2024 10:47:09 +0100
From: Simon Horman <horms@...nel.org>
To: Rodolfo Zitellini <rwz@...ro.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Doug Brown <doug@...morgal.com>
Subject: Re: [PATCH net-next 2/2] appletalk: tashtalk: Add LocalTalk line
 discipline driver for AppleTalk using a TashTalk adapter

On Sat, Aug 17, 2024 at 11:33:16AM +0200, Rodolfo Zitellini wrote:
> This is the TashTalk driver, it perits for a modern machine to
> participate in a Apple LocalTalk network and is compatibile with

nit: compatible

     Flagged by checkpatch.pl --codespell

> Netatalk.
> 
> Please see the included documentation for details:
> Documentation/networking/device_drivers/appletalk/index.rst
> 
> Signed-off-by: Rodolfo Zitellini <rwz@...ro.org>

...

> diff --git a/drivers/net/appletalk/tashtalk.c b/drivers/net/appletalk/tashtalk.c

...

> +/* Called by the driver when there's room for more data.
> + * Schedule the transmit.
> + */
> +static void tashtalk_write_wakeup(struct tty_struct *tty)
> +{
> +	struct tashtalk *tt;

I think that tt needs an __rcu annotation. Sparse says:

.../tashtalk.c:290:14: error: incompatible types in comparison expression (different address spaces):
.../tashtalk.c:290:14:    void [noderef] __rcu *
.../tashtalk.c:290:14:    void *

> +
> +	rcu_read_lock();
> +	tt = rcu_dereference(tty->disc_data);
> +	if (tt)
> +		schedule_work(&tt->tx_work);
> +	rcu_read_unlock();
> +}

...

> +static void tashtalk_send_ctrl_packet(struct tashtalk *tt, unsigned char dst,
> +				      unsigned char src, unsigned char type)
> +{
> +	unsigned char cmd = TT_CMD_TX;
> +	unsigned char buf[5];
> +	int actual;
> +	u16 crc;
> +
> +	buf[LLAP_DST_POS] = dst;
> +	buf[LLAP_SRC_POS] = src;
> +	buf[LLAP_TYP_POS] = type;
> +
> +	crc = tash_crc(buf, 3);
> +	buf[3] = ~(crc & 0xFF);
> +	buf[4] = ~(crc >> 8);
> +
> +	actual = tt->tty->ops->write(tt->tty, &cmd, 1);
> +	actual += tt->tty->ops->write(tt->tty, buf, sizeof(buf));
> +}

actual is set but otherwise unused in this function.
Should it be used as part of checking, with an error code returned on error?
If not, it should probably be removed.

Flagged by W=1 builds.

...

> +static void tashtalk_close(struct tty_struct *tty)
> +{
> +	struct tashtalk *tt = tty->disc_data;
> +
> +	/* First make sure we're connected. */
> +	if (!tt || tt->magic != TASH_MAGIC || tt->tty != tty)
> +		return;
> +
> +	spin_lock_bh(&tt->lock);
> +	rcu_assign_pointer(tty->disc_data, NULL);

I think tty->disc_data also needs an __rcu annotation, which will
likely highlight the need for annotations elsewhere. Flagged by Sparse.

> +	tt->tty = NULL;
> +	spin_unlock_bh(&tt->lock);
> +
> +	synchronize_rcu();
> +	flush_work(&tt->tx_work);
> +
> +	/* Flush network side */
> +	unregister_netdev(tt->dev);
> +	/* This will complete via tt_free_netdev */
> +}

...