netdev - Re: [PATCH v5 net-next 02/18] ionic: Add hardware init and device commands

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: <20190827195017.GR2168@lunn.ch>
Date:   Tue, 27 Aug 2019 21:50:17 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Shannon Nelson <snelson@...sando.io>
Cc:     netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH v5 net-next 02/18] ionic: Add hardware init and device
 commands

On Tue, Aug 27, 2019 at 10:39:20AM -0700, Shannon Nelson wrote:
> On 8/26/19 7:26 PM, Andrew Lunn wrote:
> >On Mon, Aug 26, 2019 at 02:33:23PM -0700, Shannon Nelson wrote:
> >>+void ionic_debugfs_add_dev(struct ionic *ionic)
> >>+{
> >>+	struct dentry *dentry;
> >>+
> >>+	dentry = debugfs_create_dir(ionic_bus_info(ionic), ionic_dir);
> >>+	if (IS_ERR_OR_NULL(dentry))
> >>+		return;
> >>+
> >>+	ionic->dentry = dentry;
> >>+}
> >Hi Shannon
> >
> >There was recently a big patchset from GregKH which removed all error
> >checking from drivers calling debugfs calls. I'm pretty sure you don't
> >need this check here.
> 
> With this check I end up either with a valid dentry value or NULL in
> ionic->dentry.  Without this check I possibly get an error value in
> ionic->dentry, which can get used later as the parent dentry to try to make
> a new debugfs node.

Hi Shannon

What you should find is that every debugfs function will have
something like:

	if (IS_ERR(dentry))
	   return dentry;
or
	if (IS_ERR(parent))
	   return parent;

If you know of a API which is missing such protection, i'm sure GregKH
would like to know. Especially since he just ripped all such
protection in driver out. Meaning he just broken some drivers if such
IS_ERR() calls are missing in the debugfs core.

> >I would be happier if there was a privacy statement, right here,
> >saying what this information is used for, and an agreement it is not
> >used for anything else. If that gets violated, you can then only blame
> >yourself when we ripe this out and hard code it to static values.
> 
> That makes perfect sense.
> 
> I can add a full description here of how the information will be used, which
> should help most folks, but I'm sure there will still be some that don't
> want this info released.
> 
> What I'd like to propose here is that I do the hardcoded strings myself for
> now, and I work up a way for the users to enable the feature as desired,
> with a reasonable comment here in the code and in the
> Documentation/.../ionic.rst file.  This might end up as an ethtool priv-flag
> that defaults to off and can set a NIC value that is remembered for later.
> 
> Does that sound reasonable?

Yes, that sounds reasonable.

Thanks
	Andrew