linux-kernel - Re: [PATCH v2 3/3] leds: trigger: implement a tty trigger

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:   Tue, 17 Dec 2019 11:58:26 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Dan Murphy <dmurphy@...com>,
        Jiri Slaby <jslaby@...e.com>, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org, linux-serial@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH v2 3/3] leds: trigger: implement a tty trigger

Hello Greg,

On Tue, Dec 17, 2019 at 09:32:11AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 17, 2019 at 09:17:18AM +0100, Uwe Kleine-König wrote:
> > +	ret = tty_get_icount(trigger_data->tty, &icount);
> > +	if (icount.rx > trigger_data->icount.rx ||
> > +	    icount.tx > trigger_data->icount.tx) {
> 
> What happens when icount.rx and/or icount.tx wraps?  It's "only" an int.

Good catch. I wonder why this is not an unsigned quantity. Just grepping
through drivers/tty/serial most drivers just increment these counters
and don't care for overflow (which is undefined for ints) either. :-\

..ooOO(Where is the can maintainer? --- We found a can of worms :-)

> > +		unsigned long delay_on = 100, delay_off = 100;
> > +
> > +		led_blink_set_oneshot(trigger_data->led_cdev,
> > +				      &delay_on, &delay_off, 0);
> > +
> > +		trigger_data->icount = icount;
> 
> Implicit memcpy of a structure?  Ick.

I'd call that elegant ;-)

> All you care about are the two integers, why not just track them instead
> of the whole thing?

For now I only care about tx and rx, but I intend to add some bells and
whistles to trigger on other events. (But I don't care much, can add
that once I implement this support.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |