netdev - Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY 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: <20181223095940.GA31681@lunn.ch>
Date:   Sun, 23 Dec 2018 10:59:40 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Marek Vasut <marex@...x.de>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH V3] net: phy: tja11xx: Add TJA11xx PHY driver

> >>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> >>> +	if (!priv->hwmon_name)
> >>> +		return -ENODEV;
> >>
> >> Do you really need to make a copy of the device name?
> >> Why not simply priv->hwmon_name = dev_name(dev) ?
> > 
> > Fine by me, but then maybe I don't quite understand why the other
> > drivers duplicate the name, eg. the sfp.c one.
> > 
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
> >> And if devm_kstrdup fails, then most likely you have an out-of-memory
> >> error, so why not return -ENOMEM as usual?
> > 
> > Fixed
> > 
> >>> +
> >>> +	for (i = 0; priv->hwmon_name[i]; i++)
> >>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> >>> +			priv->hwmon_name[i] = '_';

This is one reason to make a copy. You don't want to apply that to
main name of the device.

> >>> +
> >>> +	priv->hwmon_dev =
> >>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> >>> +						     phydev,
> >>> +						     &tja11xx_hwmon_chip_info,
> >>> +						     NULL);
> >>> +

The second reason is priv is released before dev, but what about
hwmon, especially if somebody has one of the files open? Is the
unregister synchronous?

	 Andrew