Skip to content

Commit 4e8ddd7

Browse files
w1ldptrdavem330
w1ldptr
authored and
davem330
committed
net: sched: don't release reference on action overwrite
Return from action init function with reference to action taken, even when overwriting existing action. Action init API initializes its fourth argument (pointer to pointer to tc action) to either existing action with same index or newly created action. In case of existing index(and bind argument is zero), init function returns without incrementing action reference counter. Caller of action init then proceeds working with action, without actually holding reference to it. This means that action could be deleted concurrently. Change action init behavior to always take reference to action before returning successfully, in order to protect from concurrent deletion. Reviewed-by: Marcelo Ricardo Leitner <[email protected]> Signed-off-by: Vlad Buslov <[email protected]> Signed-off-by: Jiri Pirko <[email protected]> Signed-off-by: David S. Miller <[email protected]>
1 parent 16af606 commit 4e8ddd7

17 files changed

+50
-58
lines changed

‎net/sched/act_api.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -870,8 +870,6 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
870870
}
871871
act->order = i;
872872
sz += tcf_action_fill_size(act);
873-
if (ovr)
874-
refcount_inc(&act->tcfa_refcnt);
875873
list_add_tail(&act->list, actions);
876874
}
877875

‎net/sched/act_bpf.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
311311
if (bind)
312312
return 0;
313313

314-
tcf_idr_release(*act, bind);
315-
if (!replace)
314+
if (!replace) {
315+
tcf_idr_release(*act, bind);
316316
return -EEXIST;
317+
}
317318
}
318319

319320
is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
@@ -356,8 +357,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
356357

357358
return res;
358359
out:
359-
if (res == ACT_P_CREATED)
360-
tcf_idr_release(*act, bind);
360+
tcf_idr_release(*act, bind);
361361

362362
return ret;
363363
}

‎net/sched/act_connmark.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
135135
ci = to_connmark(*a);
136136
if (bind)
137137
return 0;
138-
tcf_idr_release(*a, bind);
139-
if (!ovr)
138+
if (!ovr) {
139+
tcf_idr_release(*a, bind);
140140
return -EEXIST;
141+
}
141142
/* replacing action and zone */
142143
ci->tcf_action = parm->action;
143144
ci->zone = parm->zone;

‎net/sched/act_csum.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,18 +76,18 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
7676
} else {
7777
if (bind)/* dont override defaults */
7878
return 0;
79-
tcf_idr_release(*a, bind);
80-
if (!ovr)
79+
if (!ovr) {
80+
tcf_idr_release(*a, bind);
8181
return -EEXIST;
82+
}
8283
}
8384

8485
p = to_tcf_csum(*a);
8586
ASSERT_RTNL();
8687

8788
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
8889
if (unlikely(!params_new)) {
89-
if (ret == ACT_P_CREATED)
90-
tcf_idr_release(*a, bind);
90+
tcf_idr_release(*a, bind);
9191
return -ENOMEM;
9292
}
9393
params_old = rtnl_dereference(p->params);

‎net/sched/act_gact.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
100100
} else {
101101
if (bind)/* dont override defaults */
102102
return 0;
103-
tcf_idr_release(*a, bind);
104-
if (!ovr)
103+
if (!ovr) {
104+
tcf_idr_release(*a, bind);
105105
return -EEXIST;
106+
}
106107
}
107108

108109
gact = to_gact(*a);

‎net/sched/act_ife.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -498,12 +498,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
498498
return ret;
499499
}
500500
ret = ACT_P_CREATED;
501-
} else {
501+
} else if (!ovr) {
502502
tcf_idr_release(*a, bind);
503-
if (!ovr) {
504-
kfree(p);
505-
return -EEXIST;
506-
}
503+
kfree(p);
504+
return -EEXIST;
507505
}
508506

509507
ife = to_ife(*a);
@@ -548,6 +546,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
548546

549547
if (exists)
550548
spin_unlock_bh(&ife->tcf_lock);
549+
tcf_idr_release(*a, bind);
550+
551551
kfree(p);
552552
return err;
553553
}

‎net/sched/act_ipt.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,10 +145,11 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
145145
} else {
146146
if (bind)/* dont override defaults */
147147
return 0;
148-
tcf_idr_release(*a, bind);
149148

150-
if (!ovr)
149+
if (!ovr) {
150+
tcf_idr_release(*a, bind);
151151
return -EEXIST;
152+
}
152153
}
153154
hook = nla_get_u32(tb[TCA_IPT_HOOK]);
154155

‎net/sched/act_mirred.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
132132
if (ret)
133133
return ret;
134134
ret = ACT_P_CREATED;
135-
} else {
135+
} else if (!ovr) {
136136
tcf_idr_release(*a, bind);
137-
if (!ovr)
138-
return -EEXIST;
137+
return -EEXIST;
139138
}
140139
m = to_mirred(*a);
141140

‎net/sched/act_nat.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
6666
} else {
6767
if (bind)
6868
return 0;
69-
tcf_idr_release(*a, bind);
70-
if (!ovr)
69+
if (!ovr) {
70+
tcf_idr_release(*a, bind);
7171
return -EEXIST;
72+
}
7273
}
7374
p = to_tcf_nat(*a);
7475

‎net/sched/act_pedit.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,8 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
194194
} else {
195195
if (bind)
196196
goto out_free;
197-
tcf_idr_release(*a, bind);
198197
if (!ovr) {
198+
tcf_idr_release(*a, bind);
199199
ret = -EEXIST;
200200
goto out_free;
201201
}

‎net/sched/act_police.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,9 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
111111
if (ret)
112112
return ret;
113113
ret = ACT_P_CREATED;
114-
} else {
114+
} else if (!ovr) {
115115
tcf_idr_release(*a, bind);
116-
if (!ovr)
117-
return -EEXIST;
116+
return -EEXIST;
118117
}
119118

120119
police = to_police(*a);
@@ -195,8 +194,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
195194
failure:
196195
qdisc_put_rtab(P_tab);
197196
qdisc_put_rtab(R_tab);
198-
if (ret == ACT_P_CREATED)
199-
tcf_idr_release(*a, bind);
197+
tcf_idr_release(*a, bind);
200198
return err;
201199
}
202200

‎net/sched/act_sample.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,9 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
6969
if (ret)
7070
return ret;
7171
ret = ACT_P_CREATED;
72-
} else {
72+
} else if (!ovr) {
7373
tcf_idr_release(*a, bind);
74-
if (!ovr)
75-
return -EEXIST;
74+
return -EEXIST;
7675
}
7776
s = to_sample(*a);
7877

@@ -81,8 +80,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
8180
s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
8281
psample_group = psample_group_get(net, s->psample_group_num);
8382
if (!psample_group) {
84-
if (ret == ACT_P_CREATED)
85-
tcf_idr_release(*a, bind);
83+
tcf_idr_release(*a, bind);
8684
return -ENOMEM;
8785
}
8886
RCU_INIT_POINTER(s->psample_group, psample_group);

‎net/sched/act_simple.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
127127
} else {
128128
d = to_defact(*a);
129129

130-
tcf_idr_release(*a, bind);
131-
if (!ovr)
130+
if (!ovr) {
131+
tcf_idr_release(*a, bind);
132132
return -EEXIST;
133+
}
133134

134135
reset_policy(d, tb[TCA_DEF_DATA], parm);
135136
}

‎net/sched/act_skbedit.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
172172
ret = ACT_P_CREATED;
173173
} else {
174174
d = to_skbedit(*a);
175-
tcf_idr_release(*a, bind);
176-
if (!ovr)
175+
if (!ovr) {
176+
tcf_idr_release(*a, bind);
177177
return -EEXIST;
178+
}
178179
}
179180

180181
spin_lock_bh(&d->tcf_lock);

‎net/sched/act_skbmod.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,19 +145,17 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
145145
return ret;
146146

147147
ret = ACT_P_CREATED;
148-
} else {
148+
} else if (!ovr) {
149149
tcf_idr_release(*a, bind);
150-
if (!ovr)
151-
return -EEXIST;
150+
return -EEXIST;
152151
}
153152

154153
d = to_skbmod(*a);
155154

156155
ASSERT_RTNL();
157156
p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
158157
if (unlikely(!p)) {
159-
if (ret == ACT_P_CREATED)
160-
tcf_idr_release(*a, bind);
158+
tcf_idr_release(*a, bind);
161159
return -ENOMEM;
162160
}
163161

‎net/sched/act_tunnel_key.c

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -329,21 +329,18 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
329329
}
330330

331331
ret = ACT_P_CREATED;
332-
} else {
332+
} else if (!ovr) {
333333
tcf_idr_release(*a, bind);
334-
if (!ovr) {
335-
NL_SET_ERR_MSG(extack, "TC IDR already exists");
336-
return -EEXIST;
337-
}
334+
NL_SET_ERR_MSG(extack, "TC IDR already exists");
335+
return -EEXIST;
338336
}
339337

340338
t = to_tunnel_key(*a);
341339

342340
ASSERT_RTNL();
343341
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
344342
if (unlikely(!params_new)) {
345-
if (ret == ACT_P_CREATED)
346-
tcf_idr_release(*a, bind);
343+
tcf_idr_release(*a, bind);
347344
NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
348345
return -ENOMEM;
349346
}

‎net/sched/act_vlan.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -187,19 +187,17 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
187187
return ret;
188188

189189
ret = ACT_P_CREATED;
190-
} else {
190+
} else if (!ovr) {
191191
tcf_idr_release(*a, bind);
192-
if (!ovr)
193-
return -EEXIST;
192+
return -EEXIST;
194193
}
195194

196195
v = to_vlan(*a);
197196

198197
ASSERT_RTNL();
199198
p = kzalloc(sizeof(*p), GFP_KERNEL);
200199
if (!p) {
201-
if (ret == ACT_P_CREATED)
202-
tcf_idr_release(*a, bind);
200+
tcf_idr_release(*a, bind);
203201
return -ENOMEM;
204202
}
205203

0 commit comments

Comments
 (0)