netdev - Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters

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: <CAM_iQpX5ogcHsX+9JFxj-C8XMQmHxVM+fV+rkvv9mBK2yL2zNQ@mail.gmail.com>
Date:   Mon, 18 Feb 2019 22:42:56 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Vlad Buslov <vladbu@...lanox.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 0/5] net/sched: validate the control action with all
 the other parameters

On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@...hat.com> wrote:
>
> currently, the kernel checks for bad values of the control action in
> tcf_action_init_1(), after a successful call to the action's init()
> function. This causes three bad behaviors:

Yeah, I have been complaining about this for a long time,
although slightly differently. The problem here is the lack of
"copy" in RCU mechanism, which makes it nearly impossible
to rollback to the previous state of an action on failure path
of an update, which also makes RCU readers reading a partially
updated action too.

Before I fix the "copy" part, your fixes make sense to me. There
might be some other way to expose the action-specific tcfa_action
opcode, but it might not be better than yours.

BTW, please fold these bad behaviors into each appropriate
patch, and keep the cover letter as an overview of the whole
patchset rather than showing any details.

[...]

> all these three problems can be fixed if we validate the control action
> in the init() function, in the same way as we are already doing for all
> the other parameters.
>
> - patch 1 is a temporary fix for problem 2), but it's reverted at the
>   end of the series

Please drop patch 1, it is very unlikely only patch 1 will be backported,
I think the whole patchset should be, therefore we have no reason
to carry a temporary fix here.

Thanks.