| 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
| ||
|
Message-ID: <20190502150443.GA12003@kroah.com>
Date: Thu, 2 May 2019 17:04:43 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc: Johan Hovold <johan@...nel.org>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH v2] USB: serial: io_edgeport: mark expected switch
fall-throughs
On Thu, May 02, 2019 at 09:57:22AM -0500, Gustavo A. R. Silva wrote:
>
>
> On 5/2/19 9:47 AM, Greg Kroah-Hartman wrote:
> > On Thu, May 02, 2019 at 04:40:41PM +0200, Greg Kroah-Hartman wrote:
> >> On Thu, May 02, 2019 at 09:28:37AM -0500, Gustavo A. R. Silva wrote:
> >>>
> >>>
> >>> On 5/2/19 8:56 AM, Johan Hovold wrote:
> >>>> On Thu, May 02, 2019 at 08:22:30AM -0500, Gustavo A. R. Silva wrote:
> >>>>>
> >>>>>
> >>>>> On 5/2/19 5:26 AM, Johan Hovold wrote:
> >>>>>> On Wed, May 01, 2019 at 04:33:29PM -0500, Gustavo A. R. Silva wrote:
> >>>>>>> In preparation to enabling -Wimplicit-fallthrough, mark switch
> >>>>>>> cases where we are expecting to fall through.
> >>>>>>>
> >>>>>>> This patch fixes the following warnings:
> >>>>>>>
> >>>>>>> drivers/usb/serial/io_edgeport.c: In function ‘process_rcvd_data’:
> >>>>>>> drivers/usb/serial/io_edgeport.c:1750:7: warning: this statement may fall through [-Wimplicit-fallthrough=]
> >>>>>>> if (bufferLength == 0) {
> >>>>>>> ^
> >>>>>>> drivers/usb/serial/io_edgeport.c:1755:3: note: here
> >>>>>>> case EXPECT_HDR2:
> >>>>>>> ^~~~
> >>>>>>> drivers/usb/serial/io_edgeport.c:1810:8: warning: this statement may fall through [-Wimplicit-fallthrough=]
> >>>>>>> if (bufferLength == 0) {
> >>>>>>> ^
> >>>>>>> drivers/usb/serial/io_edgeport.c:1816:3: note: here
> >>>>>>> case EXPECT_DATA: /* Expect data */
> >>>>>>> ^~~~
> >>>>>>>
> >>>>>>> Warning level 3 was used: -Wimplicit-fallthrough=3
> >>>>>>>
> >>>>>>> Notice that, in this particular case, the code comments are modified
> >>>>>>> in accordance with what GCC is expecting to find.
> >>>>>>>
> >>>>>>> This patch is part of the ongoing efforts to enable
> >>>>>>> -Wimplicit-fallthrough.
> >>>>>>>
> >>>>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.com>
> >>>>>>> ---
> >>>>>>> Changes in v2:
> >>>>>>> - Warning level 3 is now used: -Wimplicit-fallthrough=3
> >>>>>>> instead of warning level 2.
> >>>>>>> - All warnings in the switch statement are addressed now.
> >>>>>>>
> >>>>>>> Notice that these are the last remaining fall-through warnings
> >>>>>>> in the USB subsystem. :)
> >>>>>>
> >>>>>>> drivers/usb/serial/io_edgeport.c | 3 ++-
> >>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> >>>>>>> index 4ca31c0e4174..7ad10328f4e2 100644
> >>>>>>> --- a/drivers/usb/serial/io_edgeport.c
> >>>>>>> +++ b/drivers/usb/serial/io_edgeport.c
> >>>>>>> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>>>>>> edge_serial->rxState = EXPECT_HDR2;
> >>>>>>> break;
> >>>>>>> }
> >>>>>>> - /* otherwise, drop on through */
> >>>>>>> + /* Fall through - otherwise, drop on through */
> >>>>>>> case EXPECT_HDR2:
> >>>>>>> edge_serial->rxHeader2 = *buffer;
> >>>>>>> ++buffer;
> >>>>>>> @@ -1813,6 +1813,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>>>>>> }
> >>>>>>> /* Else, drop through */
> >>>>>>> }
> >>>>>>> + /* Fall through */
> >>>>>>> case EXPECT_DATA: /* Expect data */
> >>>>>>
> >>>>>> Looks like you forgot to take the original review feedback you got into
> >>>>>> account:
> >>>>>>
> >>>>>> https://lkml.kernel.org/r/87k1zf4k24.fsf@miraculix.mork.no
> >>>>>>
> >>>>>
> >>>>> Oh, the thing is that the fall-through comments have to be placed at
> >>>>> the very bottom of the case. Also, based on that feedback, this time
> >>>>> I left the "Else, drop through" comment in place, so people can be
> >>>>> informed that such fall-through is conditional.
> >>>>>
> >>>>> What do you think about this:
> >>>>>
> >>>>> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> >>>>> index 4ca31c0e4174..52f27fc82563 100644
> >>>>> --- a/drivers/usb/serial/io_edgeport.c
> >>>>> +++ b/drivers/usb/serial/io_edgeport.c
> >>>>> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>>>> edge_serial->rxState = EXPECT_HDR2;
> >>>>> break;
> >>>>> }
> >>>>> - /* otherwise, drop on through */
> >>>>> + /* Fall through - otherwise, drop on through */
> >>>>> case EXPECT_HDR2:
> >>>>> edge_serial->rxHeader2 = *buffer;
> >>>>> ++buffer;
> >>>>> @@ -1813,6 +1813,11 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>>>> }
> >>>>> /* Else, drop through */
> >>>>> }
> >>>>> + /* Beware that, currently, there are at least three
> >>>>> + * break statements in this case block, so the
> >>>>> + * fall-through marked below is NOT unconditional.
> >>>>> + */
> >>>>> + /* Fall through */
> >>>>> case EXPECT_DATA: /* Expect data */
> >>>>> if (bufferLength < edge_serial->rxBytesRemaining) {
> >>>>> rxLen = bufferLength;
> >>>>
> >>>> It's better than v2, but I thought you said you were gonna look into
> >>>> restructuring the code to maintain (or even improve) readability?
> >>>>
> >>>
> >>> At first, I thought about that, but now I don't think that's realistic.
> >>> I'd turn the if-else into a switch, and based on the history of feedback
> >>> on this patch, we will end up having the same complains about the break
> >>> statements in that new switch and the possibility of a fall-through to
> >>> case EXPECT_DATA. At the end I would still have to add a comment explaining
> >>> that the last fall-through mark in unconditional.
> >>
> >> I love it how no one is blaming the original author of this code (i.e.
> >> me...)
> >>
> >> Let me see if I can fix it up to be more "sane", this is my fault.
> >
> > How about the following patch? Johan, this look nicer to you? It makes
> > more sense to me.
> >
>
> Thanks, Greg. Just notice that, unfortunately, the original complains are
> still applicable to your patch. :/
Really? I got rid of the crazy nesting, it should be more obvious what
is happening here now:
> > diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> > index 4ca31c0e4174..732081b3718f 100644
> > --- a/drivers/usb/serial/io_edgeport.c
> > +++ b/drivers/usb/serial/io_edgeport.c
> > @@ -1751,7 +1751,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > edge_serial->rxState = EXPECT_HDR2;
> > break;
> > }
> > - /* otherwise, drop on through */
> > + /* Fall through */
> > +
"otherwise" makes no sense here, it's obvious that if the buffer isn't
big enough we need to pass through and keep parsing.
> > case EXPECT_HDR2:
> > edge_serial->rxHeader2 = *buffer;
> > ++buffer;
> > @@ -1790,29 +1791,21 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> > edge_serial->rxHeader2, 0);
> > edge_serial->rxState = EXPECT_HDR1;
> > break;
The switch case is broken out here, so let's unindent the next block:
> > - } else {
> > - edge_serial->rxPort =
> > - IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
> > - edge_serial->rxBytesRemaining =
> > - IOSP_GET_HDR_DATA_LEN(
> > - edge_serial->rxHeader1,
> > - edge_serial->rxHeader2);
> > - dev_dbg(dev, "%s - Data for Port %u Len %u\n",
> > - __func__,
> > - edge_serial->rxPort,
> > - edge_serial->rxBytesRemaining);
> > -
> > - /* ASSERT(DevExt->RxPort < DevExt->NumPorts);
> > - * ASSERT(DevExt->RxBytesRemaining <
> > - * IOSP_MAX_DATA_LENGTH);
> > - */
> > -
> > - if (bufferLength == 0) {
> > - edge_serial->rxState = EXPECT_DATA;
> > - break;
> > - }
> > - /* Else, drop through */
> > }
> > +
> > + edge_serial->rxPort = IOSP_GET_HDR_PORT(edge_serial->rxHeader1);
> > + edge_serial->rxBytesRemaining = IOSP_GET_HDR_DATA_LEN(edge_serial->rxHeader1,
> > + edge_serial->rxHeader2);
> > + dev_dbg(dev, "%s - Data for Port %u Len %u\n", __func__,
> > + edge_serial->rxPort,
> > + edge_serial->rxBytesRemaining);
> > +
> > + if (bufferLength == 0) {
> > + edge_serial->rxState = EXPECT_DATA;
> > + break;
> > + }
> > + /* Fall through */
> > +
This should make it more obvious.
at least it is to me now, it's much easier to read, and gcc should also
be happy about it.
Parsers are a pain, you have to handle stuff like this in messy ways.
Personally, I'm horrible at parsers, but this really looks to be the
simplest way to write this code at the moment.
Johan, any thoughts?
thanks,
greg k-h
Powered by blists - more mailing lists