Re: [PATCH net-next v3 2/3] net/sched: act_vlan: No dump for unset priority

On Tue, Jun 01, 2021 at 03:35:10PM +0300, Boris Sukholitko wrote:

Hi Jacub,

On Mon, May 31, 2021 at 10:21:36PM -0700, Jakub Kicinski wrote:

quoted

On Sun, 30 May 2021 14:40:51 +0300 Boris Sukholitko wrote:

quoted

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c

[...]

quoted

quoted

@@ -362,10 +362,19 @@ static int tcf_vlan_search(struct net *net, struct tc_action **a, u32 index)
 
 static size_t tcf_vlan_get_fill_size(const struct tc_action *act)
 {
-	return nla_total_size(sizeof(struct tc_vlan))
+	struct tcf_vlan *v = to_vlan(act);
+	struct tcf_vlan_params *p;
+	size_t ret = nla_total_size(sizeof(struct tc_vlan))
 		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_ID */
-		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
-		+ nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
+		+ nla_total_size(sizeof(u16)); /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
+
+	spin_lock_bh(&v->tcf_lock);
+	p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
+	if (p->tcfv_push_prio_exists)
+		ret += nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
+	spin_unlock_bh(&v->tcf_lock);

This jumps out a little bit - if we need to take this lock to inspect
tcf_vlan_params, then I infer its value may change. And if it may
change what guarantees it doesn't change between calculating the skb
length and dumping?

It's common practice to calculate the max skb len required when
attributes are this small.

I believe you are right.

ouch, that's my fault actually - it's true, TC rules can be
modified and dumped at the same time. Then the only thing we can
do is to account for TCA_VLAN_PUSH_VLAN_PRIORITY even if we will not
fill it.

thanks for spotting this,
-- 
davide