netdev - Re: [PATCH net v2 2/2] net/sched: act_gate: fix configuration of the periodic timer

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]

Date:   Tue, 16 Jun 2020 11:20:44 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     dcaratti@...hat.com, Po Liu <Po.Liu@....com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 2/2] net/sched: act_gate: fix configuration of the
 periodic timer

On Tue, 16 Jun 2020 at 04:00, David Miller <davem@...emloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@...il.com>
> Date: Tue, 16 Jun 2020 00:55:50 +0300
>
> > On Mon, 15 Jun 2020 at 22:33, Davide Caratti <dcaratti@...hat.com> wrote:
> >>
> >> assigning a dummy value of 'clock_id' to avoid cancellation of the cycle
> >> timer before its initialization was a temporary solution, and we still
> >> need to handle the case where act_gate timer parameters are changed by
> >> commands like the following one:
> >>
> >>  # tc action replace action gate <parameters>
> >>
> >> the fix consists in the following items:
> >>
> >> 1) remove the workaround assignment of 'clock_id', and init the list of
> >>    entries before the first error path after IDR atomic check/allocation
> >> 2) validate 'clock_id' earlier: there is no need to do IDR atomic
> >>    check/allocation if we know that 'clock_id' is a bad value
> >> 3) use a dedicated function, 'gate_setup_timer()', to ensure that the
> >>    timer is cancelled and re-initialized on action overwrite, and also
> >>    ensure we initialize the timer in the error path of tcf_gate_init()
> >>
> >> v2: avoid 'goto' in gate_setup_timer (thanks to Cong Wang)
> >>
> >
> > The change log is put under the 3 '---' characters for a reason: it is
> > relevant only to reviewers, and git automatically trims it when
> > applying the patch. The way it is now, the commit message would
> > contain this line about "v2 ...".
>
> I completely disagree and I ask submitters of networking changes to keep
> the changelog in the commit message.
>
> Later people will look at this commit and ask "why didn't they do X
> or Y" and if the changelog shows that the submitter was asked not to
> do X or Y that is useful information.

Interesting, I didn't know that. If there are really relevant changes
which might matter post-review (which I don't consider "avoid 'goto'
in gate_setup_timer" to be), I usually try to integrate them better
into the main commit message. Nonetheless, sorry, feel free to ignore
me!

-Vladimir