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

Message-ID: <20191217110743.GB3055718@kroah.com>
Date:   Tue, 17 Dec 2019 12:07:43 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
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

On Tue, Dec 17, 2019 at 11:58:26AM +0100, Uwe Kleine-König wrote:
> 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. :-\

It is not undefined for the kernel, I'm pretty sure we tell the compiler
to be sane for this type of thing.  It should "just wrap".  Oh wait
"int" is "signed int" here, hah, that's funny.  Surely someone has
noticed that in 20+ years by now?

> ..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.)

Start small and add more as-needed, you can always move back to the full
structure later on if you really need it.

thanks,

greg k-h