Hi Vinicius,

On Wed, 28 Aug 2019 at 19:31, Vinicius Costa Gomes
[off-list ref] wrote:

Hi,

Vladimir Oltean [off-list ref] writes:

quoted

taprio_init may fail earlier than this line:

      list_add(&q->taprio_list, &taprio_list);

i.e. due to the net device not being multi queue.

Good catch.

quoted

Attempting to remove q from the global taprio_list when it is not part
of it will result in a kernel panic.

Fix it by iterating through the list and removing it only if found.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/sched/sch_taprio.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 540bde009ea5..f1eea8c68011 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c

@@ -1199,12 +1199,17 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,

 static void taprio_destroy(struct Qdisc *sch)
 {
-     struct taprio_sched *q = qdisc_priv(sch);
+     struct taprio_sched *p, *q = qdisc_priv(sch);
      struct net_device *dev = qdisc_dev(sch);
+     struct list_head *pos, *tmp;
      unsigned int i;

      spin_lock(&taprio_list_lock);
-     list_del(&q->taprio_list);
+     list_for_each_safe(pos, tmp, &taprio_list) {
+             p = list_entry(pos, struct taprio_sched, taprio_list);
+             if (p == q)
+                     list_del(&q->taprio_list);
+     }

Personally, I would do things differently, I am thinking: adding the
taprio instance earlier to the list in taprio_init(), and keeping
taprio_destroy() the way it is now. But take this more as a suggestion
:-)

While I don't strongly oppose your proposal (keep the list removal
unconditional, but match it better in placement to the list addition),
I think it's rather fragile and I do see this bug recurring in the
future. Anyway if you want to keep it "simpler" I can respin it like
that.

Cheers,
--
Vinicius

Regards,
-Vladimir