linux-kernel - Re: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue

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

Message-ID: <alpine.DEB.2.20.1803072039260.2814@hadrien>
Date:   Wed, 7 Mar 2018 20:41:29 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Arushi Singhal <arushisinghal19971997@...il.com>
cc:     David Miller <davem@...emloft.net>, jdmason@...zu.us,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, oss-drivers@...ronome.com,
        outreachy-kernel@...glegroups.com
Subject: Re: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary
 continue



On Wed, 7 Mar 2018, Arushi Singhal wrote:

>
>
> On Wed, Mar 7, 2018 at 9:09 PM, David Miller <davem@...emloft.net> wrote:
>       From: Arushi Singhal <arushisinghal19971997@...il.com>
>       Date: Sat, 3 Mar 2018 21:44:56 +0530
>
>       > Continue at the bottom of a loop are removed.
>       > Issue found using drop_continue.cocci Coccinelle script.
>       >
>       > Signed-off-by: Arushi Singhal
>       <arushisinghal19971997@...il.com>
>       > ---
>       > Changes in v2:
>       > - Braces is dropped from if with single statement.
>
>       Sorry there is no way I am applying this.
>
>       Just blindly removing continue statements that some static
>       checker
>       or compiler warning mentions is completely unacceptable.
>
>       Actually _LOOK_ at this code:
>
>       >               if(cards[i].vendor_id) {
>       >                       for(j=0;j<3;j++)
>       > -                           
>        if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>       {
>       > +                           
>        if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j])
>       >                                       release_region(ioaddr,
>       cards[i].total_size);
>       > -                                     continue;
>       > -                       }
>       >               }
>
>       The extraneous continue statement is the _least_ of the problems
>       here.
>
>
> Hello David
>
> Yes you are correct.
>  
>
>       This code, if the if() statement triggers, will release the
>       ioaddr
>       region and then _CONTINUE_ the for(j) loop, and try to access
>       the
>       registers using the same 'ioaddr' again.
>
>       That's actually a _REAL_ bug, and you're making it seem like
>       the behavior is intentional by editing it like this.
>
>       And the bug probably exists because this entire sequence has
>       misaligned closing curly braces.  It makes it look like
>       the continue is for the outer loop, which would be fine.
>
>       But it's not.  It's for the inner loop, and this causes the "use
>       ioaddr after releasing it" bug.
>
>
> Yes I know it's for the inner loop.
> But I am not able to find, where I am wrong here

Arushi,

You are preserving the current behavior and David is telling you that the
current behavior is incorrect.  He doesn't want a patch that changes the
code but preesrves the current (wrong) behavior, because that somehow
contributes to the illusion that the incorrect code is correct.

julia

>
> For example
> BEFORE:-
> for(i=1;i<10;i++)
>        if (expr1)
>           {
>              statement;
>              continue;
>           }
>  
> After My Patch
> for(i=1;i<10;i++)
>     if (expr1)        
>        statement;
>
> I am not understanding where I am getting wrong in understanding this.
> For me both are equivalent.
>
> I Agree with Jakub reply:-
> "This is an error handling path so the continue makes sense here to
> indicate the processing can't ever fall through if more statements are
> ever added to the loop.  But OK."
>
> Thanks
> Arushi
>
>
>       Please do not submit patches like this one, it makes for a lot
>       of
>       wasted auditing and review for people.  The onus is on you to
>       read
>       and understand the code you are editing before submitting your
>       changes.
>
>       Thank you.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@...glegroups.com.
> To post to this group, send email to outreachy-kernel@...glegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF8_axeTnahPmokxm
> Dnm4NmWo8QWSEmLejN4fO1nAiDaBA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
>