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 */
> +}

...

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ