linux-kernel - Re: [PATCH net] net: sysfs: Fix deadlock situation in sysfs accesses

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: <CANn89iK1+oLktXjHXs0U3Wo4zRZEqimoSgfPVzGGycH7R_HxnA@mail.gmail.com>
Date: Tue, 17 Dec 2024 09:16:20 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	"Eric W. Biederman" <ebiederm@...ssion.com>, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, Maxime Chevallier <maxime.chevallier@...tlin.com>, 
	TRINH THAI Florent <florent.trinh-thai@...soprasteria.com>, 
	CASAUBON Jean Michel <jean-michel.casaubon@...soprasteria.com>
Subject: Re: [PATCH net] net: sysfs: Fix deadlock situation in sysfs accesses

On Tue, Dec 17, 2024 at 8:18 AM Christophe Leroy
<christophe.leroy@...roup.eu> wrote:
>
> The following problem is encountered on kernel built with
> CONFIG_PREEMPT. An snmp daemon running with normal priority is
> regularly calling ioctl(SIOCGMIIPHY). Another process running with
> SCHED_FIFO policy is regularly reading /sys/class/net/eth0/carrier.
>
> After some random time, the snmp daemon gets preempted while holding
> the RTNL mutex then the high priority process is busy looping into
> carrier_show which bails out early due to a non-successfull
> rtnl_trylock() which implies restart_syscall(). Because the snmp
> daemon has a lower priority, it never gets the chances to release
> the RTNL mutex and the high-priority task continues to loop forever.
>
> Replace the trylock by lock_interruptible. This will increase the
> priority of the task holding the lock so that it can release it and
> allow the reader of /sys/class/net/eth0/carrier to actually perform
> its read.
>
> The problem can be reproduced with the following two simple apps:
>
> The one below runs with normal SCHED_OTHER priority:
>
>         int main(int argc, char **argv)
>         {
>                 int sk = socket(AF_INET, SOCK_DGRAM, 0);
>                 char buf[32];
>                 struct ifreq ifr = {.ifr_name = "eth0"};
>
>                 for (;;)
>                         ioctl(sk, SIOCGMIIPHY, &ifr);
>
>                 exit(0);
>         }
>
> And the following one is started with chrt -f 80 so it runs with
> SCHED_FIFO policy:
>
>         int main(int argc, char **argv)
>         {
>                 int fd = open("/sys/class/net/eth0/carrier", O_RDONLY);
>                 char buf[32];
>
>                 for (;;) {
>                         read(fd, buf, sizeof(buf));
>                         lseek(fd, 0, SEEK_SET);
>                         usleep(5000);
>                 }
>
>                 exit(0);
>         }
>
> When running alone, that high priority task takes approx 6% CPU time.
>
> When running together with the first one above, the high priority task
> reaches almost 100% of CPU time.
>
> With this fix applied, the high priority task remains at 6% CPU time
> while the other one takes the remaining CPU time available.
>
> Fixes: 336ca57c3b4e ("net-sysfs: Use rtnl_trylock in sysfs methods.")
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---

At a first glance, this might resurface the deadlock issue Eric W. Biederman
was trying to fix in 336ca57c3b4e ("net-sysfs: Use rtnl_trylock in
sysfs methods.")

I was hoping that at some point, some sysfs write methods could be
marked as : "We do not need to hold the sysfs lock"