Subject: Re: 回复: INFO: rcu detected stall in tc_modify_qdisc

From: Vinicius Costa Gomes <[email-masked]>
Date: Thu, 30 Jul 2020 10:44:12 -0700

Hi,

Dmitry Vyukov <[email-masked]> writes:

> On Wed, Jul 29, 2020 at 9:13 PM Vinicius Costa Gomes
> <[email-masked]> wrote:
>>
>> Hi,
>>
>> "Zhang, Qiang" <[email-masked]> writes:
>>
>> > ________________________________________
>> > 发件人: [email-masked] <[email-masked]> 代表 syzbot <[email-masked]>
>> > 发送时间: 2020年7月29日 13:53
>> > 收件人: [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]; [email-masked]
>> > 主题: INFO: rcu detected stall in tc_modify_qdisc
>> >
>> > Hello,
>> >
>> > syzbot found the following issue on:
>> >
>> > HEAD commit:    181964e6 fix a braino in cmsghdr_from_user_compat_to_kern()
>> > git tree:       net
>> > console output: https://syzkaller.appspot.com/x/log.txt?x=12925e38900000
>> > kernel config:  https://syzkaller.appspot.com/x/.config?x=f87a5e4232fdb267
>> > dashboard link: https://syzkaller.appspot.com/bug?extid=9f78d5c664a8c33f4cce
>> > compiler:       gcc (GCC) 10.1.0-syz 20200507
>> > syz repro:
>> > https://syzkaller.appspot.com/x/repro.syz?x=16587f8c900000
>>
>> It seems that syzkaller is generating an schedule with too small
>> intervals (3ns in this case) which causes a hrtimer busy-loop which
>> starves other kernel threads.
>>
>> We could put some limits on the interval when running in software mode,
>> but I don't like this too much, because we are talking about users with
>> CAP_NET_ADMIN and they have easier ways to do bad things to the system.
>
> Hi Vinicius,
>
> Could you explain why you don't like the argument if it's for CAP_NET_ADMIN?
> Good code should check arguments regardless I think and it's useful to
> protect root from, say, programming bugs rather than kill the machine
> on any bug and misconfiguration. What am I missing?

I admit that I am on the fence on that argument: do not let even root
crash the system (the point that my code is crashing the system gives
weight to this side) vs. root has great powers, they need to know what
they are doing.

The argument that I used to convince myself was: root can easily create
a bunch of processes and give them the highest priority and do
effectively the same thing as this issue, so I went with a the "they
need to know what they are doing side".

A bit more on the specifics here:

  - Using a small interval size, is only a limitation of the taprio
  software mode, when using hardware offloads (which I think most users
  do), any interval size (supported by the hardware) can be used;

  - Choosing a good lower limit for this seems kind of hard: something
  below 1us would never work well, I think, but things 1us < x < 100us
  will depend on the hardware/kernel config/system load, and this is the
  range includes "useful" values for many systems.

Perhaps a middle ground would be to impose a limit based on the link
speed, the interval can never be smaller than the time it takes to send
the minimum ethernet frame (for 1G links this would be ~480ns, should be
enough to catch most programming mistakes). I am going to add this and
see how it looks like.

Sorry for the brain dump :-)

>
> Also are we talking about CAP_NET_ADMIN in a user ns as well
> (effectively nobody)?

Just checked, we are talking about CAP_NET_ADMIN in user namespace as
well.

Cheers,
-- 
Vinicius