netdev - Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

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]

Date:   Tue, 20 Sep 2022 14:26:22 +0200
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add
 functionality to get RMON

On 2022-09-20 00:49, Vladimir Oltean wrote:
> On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:
>> @@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>>  	chip->rmu.smi_ops = chip->smi_ops;
>>  	chip->rmu.ds_ops = chip->ds->ops;
>>  
>> +	/* Change rmu ops with our own pointer */
>> +	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
>> +	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;
> 
> The patch splitting is still so poor, that I can't even complain about
> this bug properly, since no one uses this pointer for now (and when it
> will be used, it will not be obvious how it's assigned).
> 
> In short, it's called like this:
> 
> static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
> 					uint64_t *data)
> {
> 	struct mv88e6xxx_chip *chip = ds->priv;
> 
> 	chip->smi_ops->get_rmon(chip, port, data);
> }
> 
> When the switch doesn't support RMU operations, or when the RMU setup
> simply failed, "ethtool -S" will dereference a very NULL pointer during
> that indirect call, because mv88e6xxx_rmu_setup() is unconditionally
> called for every switch during setup. Probably not a wise choice to
> start off with the RMU ops for ethtool -S.
> 
> Also, not clear, if RMU fails, why we don't make an effort to fall back
> to MDIO for that user space request.
> 
>> +
>> +	/* Also change get stats strings for our own */
> 
> Who is "our own", and implicitly, who are they? You'd expect that there
> aren't tribalist factions within the same driver.
> 
>> +	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
>> +	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
>> +	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
>> +
> 
> Those who cast a const to a non-const pointer and proceed to modify the
> read-only structure behind it should go to patchwork arrest for one week.
> 
>>  	/* Start disabled, we'll enable RMU in master_state_change */
>>  	if (chip->info->ops->rmu_disable)
>>  		return chip->info->ops->rmu_disable(chip);
>> -- 
>> 2.25.1
>>
> 

This whole shebang was a suggestion from Andrew. I had a solution with
mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
member? I'm not really sure on how to solve this in a better way?
Suggestions any? Maybe I've misunderstood his suggestion.

/Mattias