On Tue, Jul 7, 2020 at 8:22 AM Petr Machata <pe...@mellanox.com> wrote: > > > Cong Wang <xiyou.wangc...@gmail.com> writes: > > > On Fri, Jun 26, 2020 at 3:46 PM Petr Machata <pe...@mellanox.com> wrote: > >> The function tcf_qevent_handle() should be invoked when qdisc hits the > >> "interesting event" corresponding to a block. This function releases root > >> lock for the duration of executing the attached filters, to allow packets > >> generated through user actions (notably mirred) to be reinserted to the > >> same qdisc tree. > > > > Are you sure releasing the root lock in the middle of an enqueue operation > > is a good idea? I mean, it seems racy with qdisc change or reset path, > > for example, __red_change() could update some RED parameters > > immediately after you release the root lock. > > So I had mulled over this for a while. If packets are enqueued or > dequeued while the lock is released, maybe the packet under > consideration should have been hard_marked instead of prob_marked, or > vice versa. (I can't really go to not marked at all, because the fact > that we ran the qevent is a very observable commitment to put the packet > in the queue with marking.) I figured that is not such a big deal. > > Regarding a configuration change, for a brief period after the change, a > few not-yet-pushed packets could have been enqueued with ECN marking > even as I e.g. disabled ECN. This does not seem like a big deal either, > these are transient effects.
Hmm, let's see: 1. red_enqueue() caches a pointer to child qdisc, child = q->qdisc 2. root lock is released by tcf_qevent_handle(). 3. __red_change() acquires the root lock and then changes child qdisc with a new one 4. The old child qdisc is put by qdisc_put() 5. tcf_qevent_handle() acquires the root lock again, and still uses the cached but now freed old child qdisc. Isn't this a problem? Even if it really is not, why do we make tcf_qevent_handle() callers' life so hard? They have to be very careful with race conditions like this. Thanks.
