linux-kernel - Re: [RESEND PATCHv5 09/11] modem_shm: Character device for SHM channel access.

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:	Thu, 2 Aug 2012 11:25:42 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Sjur Brændeland 
	<sjur.brandeland@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Linus Walleij <linus.walleij@...aro.org>, sjurbren@...il.com
Subject: Re: [RESEND PATCHv5 09/11] modem_shm: Character device for SHM
 channel access.

On Tue, 31 Jan 2012 09:48:44 +0100
Sjur Brændeland <sjur.brandeland@...ricsson.com> wrote:

> Add a character device implementation for the SHM stream channels.
> The character device provides asynchronous IO and ring-buffer handling.
> The device copies data directly from the Shared Memory area into
> user-land buffers.

What is the use case for this - it seems a little odd that it's not using
the tty layer so won't work with all the normal modem apps as anyone
would expect and want ?


> +static unsigned int shmchr_chrpoll(struct file *filp, poll_table *waittab)
> +{
> +	struct shmchr_char_dev *dev = filp->private_data;
> +	unsigned int mask = 0;
> +
> +	if (dev == NULL) {
> +		mdev_dbg(dev, "private_data not set!\n");
> +		return -EBADFD;
> +	}

How can this occur. If it can't occur why check ? BUG_ON() would
certainly be better to as you'd get a trace and it would get captured not
silently ignored and problems never detected.

An if.. dbg sequence to end users is basically "silently pretend we
didn't break and hope", which isn't ideal at all.


> +
> +	/* I want to be alone on dev (except status and queue). */
> +	if (mutex_lock_interruptible(&dev->mutex)) {
> +		mdev_dbg(dev, "mutex_lock_interruptible got signalled\n");
> +		mask |= POLLERR;
> +		goto out_unlocked;

That's very odd behaviour for poll() and may confuse apps. Can the mutex
ever be held for a long time ?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/