[PATCH 0/5] Taprio qdisc fixes

[PATCH 0/5] Taprio qdisc fixes

[Andre Guedes]

Hi all,

This series contains some minor improvements (patches 1 and 2) and fixes
(patches 3-5) to taprio qdisc.

Best regards,

Andre

Andre Guedes (5):
  net: sched: taprio: Remove pointless variable assigment
  net: sched: taprio: Refactor taprio_get_start_time()
  net: sched: taprio: Fix null pointer deref bug
  net: sched: taprio: Fix taprio_peek()
  net: sched: taprio: Fix taprio_dequeue()

 net/sched/sch_taprio.c | 49 +++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 20 deletions(-)

-- 
2.21.0

[PATCH 1/5] net: sched: taprio: Remove pointless variable assigment

[Andre Guedes]

This patch removes a pointless variable assigment in taprio_change().
The 'err' variable is not used from this assignment to the next one so
this patch removes it.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 net/sched/sch_taprio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 1b0fb80162e6..3214a65775f3 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -646,7 +646,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	if (err < 0)
 		return err;
 
-	err = -EINVAL;
 	if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
 		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
 
-- 
2.21.0

[PATCH 2/5] net: sched: taprio: Refactor taprio_get_start_time()

[Andre Guedes]

This patch does a code refactoring to taprio_get_start_time() function
to improve readability and report error properly.

If 'base' time is later than 'now', the start time is equal to 'base'
and taprio_get_start_time() is done. That's the natural case so we move
that code to the beginning of the function. Also, if 'cycle' calculation
is zero, something went really wrong with taprio and we should log that
internal error properly.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 net/sched/sch_taprio.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 3214a65775f3..f7139e6179b6 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -532,7 +532,7 @@ static int taprio_parse_mqprio_opt(struct net_device *dev,
 	return 0;
 }
 
-static ktime_t taprio_get_start_time(struct Qdisc *sch)
+static int taprio_get_start_time(struct Qdisc *sch, ktime_t *start)
 {
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct sched_entry *entry;
@@ -540,27 +540,33 @@ static ktime_t taprio_get_start_time(struct Qdisc *sch)
 	s64 n;
 
 	base = ns_to_ktime(q->base_time);
-	cycle = 0;
+	now = q->get_time();
+
+	if (ktime_after(base, now)) {
+		*start = base;
+		return 0;
+	}
 
 	/* Calculate the cycle_time, by summing all the intervals.
 	 */
+	cycle = 0;
 	list_for_each_entry(entry, &q->entries, list)
 		cycle = ktime_add_ns(cycle, entry->interval);
 
-	if (!cycle)
-		return base;
-
-	now = q->get_time();
-
-	if (ktime_after(base, now))
-		return base;
+	/* The qdisc is expected to have at least one sched_entry.  Moreover,
+	 * any entry must have 'interval' > 0. Thus if the cycle time is zero,
+	 * something went really wrong. In that case, we should warn about this
+	 * inconsistent state and return error.
+	 */
+	if (WARN_ON(!cycle))
+		return -EFAULT;
 
 	/* Schedule the start time for the beginning of the next
 	 * cycle.
 	 */
 	n = div64_s64(ktime_sub_ns(now, base), cycle);
-
-	return ktime_add_ns(base, (n + 1) * cycle);
+	*start = ktime_add_ns(base, (n + 1) * cycle);
+	return 0;
 }
 
 static void taprio_start_sched(struct Qdisc *sch, ktime_t start)
@@ -711,9 +717,12 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	}
 
 	taprio_set_picos_per_byte(dev, q);
-	start = taprio_get_start_time(sch);
-	if (!start)
-		return 0;
+
+	err = taprio_get_start_time(sch, &start);
+	if (err < 0) {
+		NL_SET_ERR_MSG(extack, "Internal error: failed get start time");
+		return err;
+	}
 
 	taprio_start_sched(sch, start);
 
-- 
2.21.0

[PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Andre Guedes]

If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
generating a null pointer dereference bug. This patch fixes should_
restart_cycle() so we return if the pointer is NULL.

Fixes: 5a781ccbd19e (“tc: Add support for configuring the taprio scheduler”)
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 net/sched/sch_taprio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index f7139e6179b6..1671510c187f 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -204,7 +204,8 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
 static bool should_restart_cycle(const struct taprio_sched *q,
 				 const struct sched_entry *entry)
 {
-	WARN_ON(!entry);
+	if (WARN_ON(!entry))
+		return false;
 
 	return list_is_last(&entry->list, &q->entries);
 }
-- 
2.21.0

[PATCH 4/5] net: sched: taprio: Fix taprio_peek()

[Andre Guedes]

While traversing taprio's children qdisc list, if the gate is closed for
a given traffic class, we should continue traversing the list since the
remaining qdiscs may have skb ready for transmission.

This patch also takes this opportunity and changes the function to use
the TAPRIO_ALL_GATES_OPEN macro instead of the magic number '-1'.

Fixes: 5a781ccbd19e (“tc: Add support for configuring the taprio scheduler”)
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 net/sched/sch_taprio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 1671510c187f..110436d11f86 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -89,7 +89,7 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch)
 
 	rcu_read_lock();
 	entry = rcu_dereference(q->current_entry);
-	gate_mask = entry ? entry->gate_mask : -1;
+	gate_mask = entry ? entry->gate_mask : TAPRIO_ALL_GATES_OPEN;
 	rcu_read_unlock();
 
 	if (!gate_mask)
@@ -111,7 +111,7 @@ static struct sk_buff *taprio_peek(struct Qdisc *sch)
 		tc = netdev_get_prio_tc_map(dev, prio);
 
 		if (!(gate_mask & BIT(tc)))
-			return NULL;
+			continue;
 
 		return skb;
 	}
-- 
2.21.0

[PATCH 5/5] net: sched: taprio: Fix taprio_dequeue()

[Andre Guedes]

In case we don't have 'guard' or 'budget' to transmit the skb, we should
continue traversing the qdisc list since the remaining guard/budget
might be enough to transmit a skb from other children qdiscs.

Fixes: 5a781ccbd19e (“tc: Add support for configuring the taprio scheduler”)
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 net/sched/sch_taprio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 110436d11f86..9f361f2b7510 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -180,12 +180,12 @@ static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
 		 */
 		if (gate_mask != TAPRIO_ALL_GATES_OPEN &&
 		    ktime_after(guard, entry->close_time))
-			return NULL;
+			continue;
 
 		/* ... and no budget. */
 		if (gate_mask != TAPRIO_ALL_GATES_OPEN &&
 		    atomic_sub_return(len, &entry->budget) < 0)
-			return NULL;
+			continue;
 
 		skb = child->ops->dequeue(child);
 		if (unlikely(!skb))
-- 
2.21.0

Re: [PATCH 0/5] Taprio qdisc fixes

[David Miller]

From: Andre Guedes <andre.guedes@intel.com>
Date: Fri, 19 Apr 2019 17:00:47 -0700

> This series contains some minor improvements (patches 1 and 2) and
> fixes (patches 3-5) to taprio qdisc.

This series does not apply cleanly at all to the net tree.

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Cong Wang]

On Fri, Apr 19, 2019 at 6:06 PM Andre Guedes <andre.guedes@intel.com> wrote:
>
> If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
> generating a null pointer dereference bug. This patch fixes should_
> restart_cycle() so we return if the pointer is NULL.

Hmm, while you are on it, how is it possible to have entry==NULL
for should_restart_cycle(). It is only called in advance_sched()
where entry is already checked against NULL right before it,
so for me, entry is always NULL at the point of calling
should_restart_cycle().

Am I missing anything?

Thanks.

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Cong Wang]

On Mon, Apr 22, 2019 at 11:04 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Apr 19, 2019 at 6:06 PM Andre Guedes <andre.guedes@intel.com> wrote:
> >
> > If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
> > generating a null pointer dereference bug. This patch fixes should_
> > restart_cycle() so we return if the pointer is NULL.
>
> Hmm, while you are on it, how is it possible to have entry==NULL
> for should_restart_cycle(). It is only called in advance_sched()
> where entry is already checked against NULL right before it,
> so for me, entry is always NULL at the point of calling
> should_restart_cycle().

I meant, 'entry' is always non-NULL here... I typed too fast.

Re: [PATCH 0/5] Taprio qdisc fixes

[Guedes, Andre]

Hi David,

> On Apr 20, 2019, at 8:46 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: Andre Guedes <andre.guedes@intel.com>
> Date: Fri, 19 Apr 2019 17:00:47 -0700
> 
>> This series contains some minor improvements (patches 1 and 2) and
>> fixes (patches 3-5) to taprio qdisc.
> 
> This series does not apply cleanly at all to the net tree.

This series in based on top of net-next tree. I just realized I missed adding the “net-next” prefix to the subject.

Do you want me to re-send it with the “net-next” prefix?

Thanks,

Andre

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Guedes, Andre]

> On Apr 22, 2019, at 11:07 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> On Mon, Apr 22, 2019 at 11:04 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> 
>> On Fri, Apr 19, 2019 at 6:06 PM Andre Guedes <andre.guedes@intel.com> wrote:
>>> 
>>> If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
>>> generating a null pointer dereference bug. This patch fixes should_
>>> restart_cycle() so we return if the pointer is NULL.
>> 
>> Hmm, while you are on it, how is it possible to have entry==NULL
>> for should_restart_cycle(). It is only called in advance_sched()
>> where entry is already checked against NULL right before it,
>> so for me, entry is always NULL at the point of calling
>> should_restart_cycle().
> 
> I meant, 'entry' is always non-NULL here... I typed too fast.

Your assessment is correct. I believe the WARN_ON() was added as a defensive practice to prevent null pointer dereference in case someone misuse that helper in the future.

- Andre

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Cong Wang]

On Mon, Apr 22, 2019 at 12:24 PM Guedes, Andre <andre.guedes@intel.com> wrote:
>
>
> > On Apr 22, 2019, at 11:07 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Mon, Apr 22, 2019 at 11:04 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >>
> >> On Fri, Apr 19, 2019 at 6:06 PM Andre Guedes <andre.guedes@intel.com> wrote:
> >>>
> >>> If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
> >>> generating a null pointer dereference bug. This patch fixes should_
> >>> restart_cycle() so we return if the pointer is NULL.
> >>
> >> Hmm, while you are on it, how is it possible to have entry==NULL
> >> for should_restart_cycle(). It is only called in advance_sched()
> >> where entry is already checked against NULL right before it,
> >> so for me, entry is always NULL at the point of calling
> >> should_restart_cycle().
> >
> > I meant, 'entry' is always non-NULL here... I typed too fast.
>
> Your assessment is correct. I believe the WARN_ON() was added as a defensive practice to prevent null pointer dereference in case someone misuse that helper in the future.

Yeah, so we can just remove it. :)

Re: [PATCH 0/5] Taprio qdisc fixes

[David Miller]

From: "Guedes, Andre" <andre.guedes@intel.com>
Date: Mon, 22 Apr 2019 19:18:26 +0000

>> On Apr 20, 2019, at 8:46 PM, David Miller <davem@davemloft.net> wrote:
>> 
>> From: Andre Guedes <andre.guedes@intel.com>
>> Date: Fri, 19 Apr 2019 17:00:47 -0700
>> 
>>> This series contains some minor improvements (patches 1 and 2) and
>>> fixes (patches 3-5) to taprio qdisc.
>> 
>> This series does not apply cleanly at all to the net tree.
> 
> This series in based on top of net-next tree. I just realized I missed adding the “net-next” prefix to the subject.
> 
> Do you want me to re-send it with the “net-next” prefix?

Yes, please.

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Guedes, Andre]

> On Apr 22, 2019, at 12:36 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> On Mon, Apr 22, 2019 at 12:24 PM Guedes, Andre <andre.guedes@intel.com> wrote:
>> 
>> 
>>> On Apr 22, 2019, at 11:07 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> 
>>> On Mon, Apr 22, 2019 at 11:04 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> 
>>>> On Fri, Apr 19, 2019 at 6:06 PM Andre Guedes <andre.guedes@intel.com> wrote:
>>>>> 
>>>>> If 'entry' is NULL we WARN_ON() but dereference the pointer anyway,
>>>>> generating a null pointer dereference bug. This patch fixes should_
>>>>> restart_cycle() so we return if the pointer is NULL.
>>>> 
>>>> Hmm, while you are on it, how is it possible to have entry==NULL
>>>> for should_restart_cycle(). It is only called in advance_sched()
>>>> where entry is already checked against NULL right before it,
>>>> so for me, entry is always NULL at the point of calling
>>>> should_restart_cycle().
>>> 
>>> I meant, 'entry' is always non-NULL here... I typed too fast.
>> 
>> Your assessment is correct. I believe the WARN_ON() was added as a defensive practice to prevent null pointer dereference in case someone misuse that helper in the future.
> 
> Yeah, so we can just remove it. :)

Fine by me. In that case, the function should_restart_cycle() will be just a dummy wrapper on list_is_last() so we should probably get rid of it and call list_is_last() within advance_sched().

Re: [PATCH 3/5] net: sched: taprio: Fix null pointer deref bug

[Cong Wang]

On Tue, Apr 23, 2019 at 12:21 PM Guedes, Andre <andre.guedes@intel.com> wrote:
>
>
> > On Apr 22, 2019, at 12:36 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > Yeah, so we can just remove it. :)
>
> Fine by me. In that case, the function should_restart_cycle() will be just a dummy wrapper on list_is_last() so we should probably get rid of it and call list_is_last() within advance_sched().

Yes, agreed.

Thanks.