netdev - question lan9303: DSA concurrency and locking,

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-next>] [day] [month] [year] [list]

Message-ID: <47810c8e-841a-a1a6-8957-6f4114dad380@egil-hjelmeland.no>
Date:   Wed, 15 Nov 2017 13:08:22 +0100
From:   Egil Hjelmeland <privat@...l-hjelmeland.no>
To:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev <netdev@...r.kernel.org>
Subject: question lan9303: DSA concurrency and locking,

Hi experts

I am hoping for some guidance.

Does DSA offer any protection against concurrent calls of dsa_switch_ops?

I did not include any locking in the code I contributed to lan9303. 
First I felt bad locking is worse than no locking. Second I assumed that 
reviewers would point out if locking is needed.

The most "interesting" part of the lan9303 driver that has no locking is 
the ALR (=fdb/mdb). ALR access is a sequence of register operations. 
Anyway it is very unlikely that mdb related calls are reentered. But if 
it can happen, it would mean that IGMP snooping can go wrong. (Which is 
actually very bad in our applications.)

Is this something to worry about?

If yes, what is the best strategy to deal with it?

Looking in drivers/net/dsa/lan9303-core.c :

lan9303_indirect_phy_read/lan9303_indirect_phy_write + 
lan9303_write_switch_reg/lan9303_read_switch_reg are already protected 
by chip->indirect_mutex

ALR access uses lan9303_write_switch_reg/lan9303_read_switch_reg.

Adding a chip->alr_mutex on top of chip->indirect_mutex does not feel 
right to me. Would it be better to move the locking up to the relevant 
dsa_switch_ops functions?

Thanks

Egil Hjelmeland