netdev - Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 03 Jan 2018 10:43:54 -0500
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Russell King <rmk+kernel@....linux.org.uk>
Subject: Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

> On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@...n.ch> writes:
>> 
>> > -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> > +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  {
>> >  	struct mv88e6xxx_chip *chip = ds->priv;
>> >  
>> > @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  	return 0;
>> >  }
>> 
>> We worked to remove the old underscore prefix convention. Please don't
>> add it back... Simply rework the return statements of
>> mv88e6xxx_get_sset_count to lock/unlock there.
>
> That makes mv88e6xxx_get_sset_count quite complex, making it error
> prone. Doing the locking in a separate function makes is very clear
> the lock is held and then correctly released. So i will just rename
> _mv88e6xxx_get_sset_count() to mv88e6xxx_get_sset_count_locked()

     static int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
     {
            struct mv88e6xxx_chip *chip = ds->priv;
    +       int err;
     
    -       if (chip->info->ops->stats_get_sset_count)
    -               return chip->info->ops->stats_get_sset_count(chip);
    +       if (!chip->info->ops->stats_get_sset_count)
    +               return 0;
     
    -       return 0;
    +       mutex_lock(&chip->reg_lock);
    +       err = chip->info->ops->stats_get_sset_count(chip);
    +       mutex_unlock(&chip->reg_lock);
    +
    +       return err;
     }


This is quite complex and error prone, seriously?


     Vivien