netdev - Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64

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: <20201203100121.64bb2774@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date:   Thu, 3 Dec 2020 10:01:21 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Russell King <linux@...linux.org.uk>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mips@...r.kernel.org
Subject: Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64

On Thu, 3 Dec 2020 18:53:20 +0100 Oleksij Rempel wrote:
> On Thu, Dec 03, 2020 at 08:35:17AM -0800, Jakub Kicinski wrote:
> > On Thu, 3 Dec 2020 09:50:11 +0100 Oleksij Rempel wrote:  
> > > @Jakub,
> > >   
> > > > You can't take sleeping locks from .ndo_get_stats64.
> > > > 
> > > > Also regmap may sleep?
> > > > 
> > > > +	ret = regmap_read(priv->regmap, reg, &val);    
> > > 
> > > Yes. And underling layer is mdio bus which is by default sleeping as
> > > well.
> > >   
> > > > Am I missing something?    
> > > 
> > > In this log, the  ar9331_get_stats64() was never called from atomic or
> > > irq context. Why it should not be sleeping?  
> > 
> > You missed some long discussions about this within last week on netdev.
> > Also Documentation/networking/statistics.rst.
> > 
> > To answer your direct question - try:
> > 
> > # cat /proc/net/dev
> > 
> > procfs iterates over devices while holding only an RCU read lock.  
> 
> Now i can reproduce it :)
> 
> [33683.199864] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:935
> [33683.210737] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 593, name: cat
> [33683.216796] INFO: lockdep is turned off.
> [33683.222972] CPU: 0 PID: 593 Comm: cat Not tainted 5.10.0-rc3-ar9331-00733-gff7090915bb7-dirty #28
> [33683.231743] Stack : 808f0000 80885ffc 820eba5c 00000000 00000000 d4a19200 80980000 819a93c8
> [33683.240093]         80980ca7 80d43358 804ee1f4 80980000 00000002 800afe08 820eba08 d4a19200
> [33683.247181]         00000000 00000000 8089ffb0 00000000 820ebfe0 00000000 00000000 00000000
> [33683.257767]         820ebab4 77bbfdc0 00fae587 77e859a0 80980000 80000000 00000000 80990000
> [33683.266107]         804ee1f4 80980000 00000002 8200f750 8097ca9c d4a19200 000859df 00000001
> [33683.274529]         ...
> [33683.275626] Call Trace:
> [33683.280156] [<80069ce0>] show_stack+0x9c/0x140
> [33683.283200] [<800afe08>] ___might_sleep+0x220/0x244
> [33683.290441] [<8073c030>] __mutex_lock+0x70/0x374
> [33683.293651] [<8073c360>] mutex_lock_nested+0x2c/0x38
> [33683.300793] [<804ee1f4>] ar9331_read_stats+0x34/0x834
> [33683.304441] [<804eea34>] ar9331_get_stats64+0x40/0x394
> [33683.311797] [<80526584>] dev_get_stats+0x58/0xfc
> [33683.315013] [<805657bc>] dev_seq_printf_stats+0x44/0x228
> [33683.322476] [<805659e8>] dev_seq_show+0x48/0x50
> [33683.325601] [<8021dd28>] seq_read_iter+0x3d8/0x4d0
> [33683.332585] [<8021df60>] seq_read+0x140/0x198
> [33683.335532] [<8026f950>] proc_reg_read+0xe4/0xf8
> [33683.342397] [<801f0840>] vfs_read+0xc8/0x1a8
> [33683.345260] [<801f0b7c>] ksys_read+0x9c/0xfc
> [33683.352056] [<80071aa4>] syscall_common+0x34/0x58
> 
> Hm.. There is no way i can guarantee that underlying mdio system is
> not using mutexes. So, i can't read stats directly from HW within
> ar9331_get_stats64(), only driver internal storage can be used. It is possible
> to poll it more frequently, but  it make no reals sense on this low power
> devices.
> 
> What kind of options do we have?

Vladimir has been looking at solving this, I'll let him answer with his
latest thoughts.