linux-kernel - RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

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]

Message-ID: <BLUPR12MB0420CA022C20D8EDA7D8F563EBF50@BLUPR12MB0420.namprd12.prod.outlook.com>
Date:	Thu, 7 Jan 2016 02:53:55 +0000
From:	"Yu, Xiangliang" <Xiangliang.Yu@....com>
To:	Jon Mason <jdmason@...zu.us>, "Hubbe, Allen" <Allen.Hubbe@....com>
CC:	Dave Jiang <dave.jiang@...el.com>,
	"linux-ntb@...glegroups.com" <linux-ntb@...glegroups.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	SPG_Linux_Kernel <SPG_Linux_Kernel@....com>
Subject: RE: [PATCH V2 1/3] NTB: Add AMD PCI-Express NTB driver

> >
> >> > +#define ndev_pdev(ndev) ((ndev)->ntb.pdev) #define
> ndev_name(ndev)
> >> > +pci_name(ndev_pdev(ndev)) #define ndev_dev(ndev)
> >> > +(&ndev_pdev(ndev)->dev) #define ntb_ndev(ntb) container_of(ntb,
> >> > +struct amd_ntb_dev, ntb) #define hb_ndev(work) container_of(work,
> >> > +struct amd_ntb_dev,
> >> hb_timer.work)
> >> > +#define ntb_hotplug_ndev(context) (container_of((context),     \
> >> > +                       struct ntb_acpi_hotplug_context, hp)->ndev)
> >>
> >> Seems like these are hiding things too.  Please use them directly (or
> >> at least put them in the C file and not the header file).
> >
> > I like these macros for up/down casting.  Putting them close to the
> structure definition seems appropriate to me, too.  I would rather see them
> moved to right below the definition of struct amd_ntb_dev, instead of to the
> c file.  That is my opinion, but Jon can make the final decision on it.
> 
> My opinion wasn't super strong on these.  If Allen is fine with them, then
> good enough for me :)

Agree with Allen's opinion.

> 
> > However, these in particular are buggy:
> >
> >> > +#define ntb_ndev(ntb) container_of(ntb, struct amd_ntb_dev, ntb)
> >> > +#define hb_ndev(work) container_of(work, struct amd_ntb_dev,
> >> hb_timer.work)
> >
> > Note: "ntb" will be replaced in all occurrences to the right.  This only works
> if the name "ntb" is passed as the argument.  If the argument is named "foo",
> it will either fail at compile time to find the member "foo" in struct
> amd_ntb_dev, or worse, it will hide a bug accessing the wrong member of
> the struct.
> >
> > Rename the macro parameter __ntb.
> 
> Good call.  Please make the necessary mods Xiangliang.

Ok