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

[Eric Dumazet]

On Tue, Dec 17, 2024 at 8:18 AM Christophe Leroy
<christophe.leroy@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---

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"