netdev - Re: [PATCH net-next 16/17] net: sched: conditionally take rtnl lock on rules update path

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20181113104016.76d12436@redhat.com>
Date:   Tue, 13 Nov 2018 10:40:16 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, davem@...emloft.net, ast@...nel.org,
        daniel@...earbox.net
Subject: Re: [PATCH net-next 16/17] net: sched: conditionally take rtnl lock
 on rules update path

Hi Vlad,

On Mon, 12 Nov 2018 09:55:45 +0200
Vlad Buslov <vladbu@...lanox.com> wrote:

> @@ -179,9 +179,25 @@ static void tcf_proto_destroy_work(struct work_struct *work)
>  	rtnl_unlock();
>  }
>  
> +/* Helper function to lock rtnl mutex when specified condition is true and mutex
> + * hasn't been locked yet. Will set rtnl_held to 'true' before taking rtnl lock.
> + * Note that this function does nothing if rtnl is already held. This is
> + * intended to be used by cls API rules update API when multiple conditions
> + * could require rtnl lock and its state needs to be tracked to prevent trying
> + * to obtain lock multiple times.
> + */
> +
> +static void tcf_require_rtnl(bool cond, bool *rtnl_held)
> +{
> +	if (!*rtnl_held && cond) {
> +		*rtnl_held = true;
> +		rtnl_lock();
> +	}
> +}

I guess calls to this function are supposed to be serialised. If that's
the case (which is my tentative understanding so far), I would indicate
that in the comment.

If that's not the case, you would be introducing a race I guess.

Same applies to tcf_block_release() from 17/17.

-- 
Stefano