netdev - Re: [PATCH] net/sched/sch_hfsc.c: initialize parent's cl_cfmin properly in init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4C913DE1.6040401@ziu.info>
Date:	Wed, 15 Sep 2010 23:42:57 +0200
From:	Michal Soltys <soltys@....info>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	David Miller <davem@...emloft.net>, denys@...p.net.lb,
	netdev@...r.kernel.org
Subject: Re: [PATCH] net/sched/sch_hfsc.c: initialize parent's cl_cfmin properly
 in init_vf()

On 10-09-15 19:54, Patrick McHardy wrote:
> Am 01.09.2010 23:30, schrieb David Miller:
>>  From: Michal Soltys<soltys@....info>
>>  Date: Mon, 30 Aug 2010 23:34:10 +0200
>>
>>>  This patch fixes init_vf() function, so on each new backlog period parent's
>>>  cl_cfmin is properly updated (including further propgation towards the root),
>>>  even if the activated leaf has no upperlimit curve defined.
>>>
>>>  Signed-off-by: Michal Soltys<soltys@....info>
>>
>>  Applied, thanks.
>
> For the record, the patch seems fine to me. The root cause seems to be
> an optimization I introduced (pre-git, even history.git unfortunately)
> in vttree_get_minvt() that wasn't present in the original version:
>
> static struct hfsc_class *
> vttree_get_minvt(struct hfsc_class *cl, u64 cur_time)
> {
>          /* if root-class's cfmin is bigger than cur_time nothing to do */
>          if (cl->cl_cfmin>  cur_time)
>                  return NULL;
>
> I'd prefer to remove this check since it's obviously not correct and
> might cause other problems. Michal, could you please test whether this
> patch fixes the problem as well? Thanks!

Sure, will do. Although with a tad bit more complex scenario than from 
my cover email, I think there will still be problems. For example:

     A
    / \
   B   C(u2)
  / \
D   E(u1)

C and E are constantly backlogged, D becomes active. It will add itself 
to B's vt_tree, but won't update B's cl_cfmin (without my earlier fix). 
B is already active, its myf and cfmin didn't change, so neither did f. 
And so nothing will change for A either.

Now we try a dequeue. Without your old optimization (btw I think it's a 
good shortcut, if I didn't miss anything), A will choose min vt from B 
and C, if their f <= cur_time. B's f should be at this moment 0 (D has 
no upperlimit and became active), but it remained unchanged. So we have 
the same problem as previously. This would work with a trivial hierarchy 
though, such as:

     A
    / \
   B   C(u2)

But deeper scenarios seem problematic. Of course I'll do the actual 
tests to be sure.

A bit more constrained version from my patch (to limit update_cfmin() 
calls) could be:
                 f = max(cl->cl_myf, cl->cl_cfmin);
                 if (f != cl->cl_f) {
                         cl->cl_f = f;
                         cftree_update(cl);
                         update_cfmin(cl->cl_parent);
                 } else if (go_active)
                         update_cfmin(cl->cl_parent);

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html