linux-kernel - Re: [PATCH] staging: dgnc: replace DGNC_VERIFY

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]

Date:   Tue, 18 Oct 2016 08:40:18 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Fernando Apesteguía 
        <fernando.apesteguia@...il.com>
Cc:     devel@...verdev.osuosl.org, Lidza Louina <lidza.louina@...il.com>,
        driverdev-devel@...uxdriverproject.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: dgnc: replace DGNC_VERIFY_BOARD macro

On Mon, Oct 17, 2016 at 11:46:36PM +0200, Fernando Apesteguía wrote:
> On Mon, Oct 17, 2016 at 10:29 AM, Greg KH <gregkh@...uxfoundation.org> wrote:
> > On Fri, Oct 14, 2016 at 07:22:57PM +0200, Fernando Apesteguia wrote:
> >> The patch replaces the macro with a function (dgnc_get_board) and
> >> substitutes the macro statement with a call to that function and a
> >> comparison on the returned value.
> >>
> >> This removes a checkpatch warning.
> >>
> >> Signed-off-by: Fernando Apesteguia <fernando.apesteguia@...il.com>
> >> ---
> >>  drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
> >>  1 file changed, 51 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/staging/dgnc/dgnc_sysfs.c b/drivers/staging/dgnc/dgnc_sysfs.c
> >> index 290bf6e..3ea23a9 100644
> >> --- a/drivers/staging/dgnc/dgnc_sysfs.c
> >> +++ b/drivers/staging/dgnc/dgnc_sysfs.c
> >> @@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
> >>       driver_remove_file(driverfs, &driver_attr_pollrate);
> >>  }
> >>
> >> -#define DGNC_VERIFY_BOARD(p, bd)                             \
> >> -     do {                                                    \
> >> -             if (!p)                                         \
> >> -                     return 0;                               \
> >> -                                                             \
> >> -             bd = dev_get_drvdata(p);                        \
> >> -             if (!bd || bd->magic != DGNC_BOARD_MAGIC)       \
> >> -                     return 0;                               \
> >> -             if (bd->state != BOARD_READY)                   \
> >> -                     return 0;                               \
> >> -     } while (0)
> >> +static struct dgnc_board *dgnc_get_board(struct device *p)
> >> +{
> >> +     struct dgnc_board *bd;
> >> +
> >> +     if (!p)
> >> +             return NULL;
> >> +
> >> +     bd = dev_get_drvdata(p);
> >> +     if (!bd || bd->magic != DGNC_BOARD_MAGIC)
> >> +             return NULL;
> >> +     if (bd->state != BOARD_READY)
> >> +             return NULL;
> >> +
> >> +     return bd;
> >> +}
> >
> > No, this macro should be removed entirely as what it does is pointless
> > in some parts, wrong in others, and not needed at all in the rest :(
> >
> > I've asked others to fix this up properly in the past, but it doesn't
> > seem like anyone wants to do the work...
> >
> 
> I tried to find the discussion the relevant mails in lkml.org but
> couldn't find them. Could  you point me to them so I can have a look?

Last time I mentioned this, it was on the outreachy-kernel mailing list.

Just walk through the code for yourself and see which, if any, of these
things could ever actually cause the function to "fail".  I think you
will find that none of them can ever happen...

The first test is a huge proof that the original author didn't
understand how sysfs or "container_of()" works, given that it is
impossible to ever have happen.

thanks,

greg k-h