linux-kernel - Re: [PATCH 1/1] mct_u232: added _ioctl and _get

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] [day] [month] [year] [list]

Message-ID: <20101226205756.78f4d21a@lembas.zaitcev.lan>
Date:	Sun, 26 Dec 2010 20:57:56 -0700
From:	Pete Zaitcev <zaitcev@...hat.com>
To:	Tsozik <tsozik@...oo.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, zaitcev@...hat.com
Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions

On Sun, 26 Dec 2010 18:58:46 -0800 (PST)
Tsozik <tsozik@...oo.com> wrote:

> Please let me know if there's anything else I need to correct before
> it can be rolled out,

I am hardly an expert here, but I looked it up as much as I could,
and this looks like a wrong way to do it:

> @@ -386,27 +396,41 @@ static int mct_u232_get_modem_stat(struc
>  	/* Translate Control Line states */
> +	if (msr & MCT_U232_MSR_DSR) {
>  		*control_state |=  TIOCM_DSR;
> +		icount->dsr++;
> +	} else {
>  		*control_state &= ~TIOCM_DSR;
> +	}

Unfortunately, Kerrisk's manpages do not seem to offer an authoritative
definition, but the expectations across the code seems that the counter
actually counts the changes. I know that some people talked about
counting "interrupts", but the way it's implemented in trustworthy
drivers suggests something like this:

	unsigned int old_ctl_state;

	old_ctl_state = *control_state;
	if (msr & MCT_U232_MSR_DSR)
		*control_state |=  TIOCM_DSR;
	else
		*control_state &= ~TIOCM_DSR;
	if (old_ctl_state != *control_state)
		icount->dsr++;
	...... repeat for all bits

E.g. for DSR going down too, although perhaps I'm not suggesting the
best way to do it. Could you re-implement it like the above and
check that your radiation counter works the same with it and
when driven by a built-in serial port?

Another thing, you should take priv->lock around fetching of
mct_u232_port->icount in mct_u232_ioctl. It seems superfluous,
I know, because the API itself is racy, but just to be nice
and doing the expected thing...

-- Pete
--
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/