Skip to content

Commit 0190c1d

Browse files
w1ldptrdavem330
w1ldptr
authored and
davem330
committed
net: sched: atomically check-allocate action
Implement function that atomically checks if action exists and either takes reference to it, or allocates idr slot for action index to prevent concurrent allocations of actions with same index. Use EBUSY error pointer to indicate that idr slot is reserved. Implement cleanup helper function that removes temporary error pointer from idr. (in case of error between idr allocation and insertion of newly created action to specified index) Refactor all action init functions to insert new action to idr using this API. Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Signed-off-by: Vlad Buslov <vladbu@mellanox.com> Signed-off-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent cae422f commit 0190c1d

File tree

18 files changed

+216
-59
lines changed

18 files changed

+216
-59
lines changed

‎include/net/act_api.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
154154
int bind, bool cpustats);
155155
void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
156156

157+
void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
158+
int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
159+
struct tc_action **a, int bind);
157160
int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
158161
int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
159162

‎net/sched/act_api.c‎

Lines changed: 72 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,9 @@ static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
303303

304304
spin_lock(&idrinfo->lock);
305305
p = idr_find(&idrinfo->action_idr, index);
306-
if (p) {
306+
if (IS_ERR(p)) {
307+
p = NULL;
308+
} else if (p) {
307309
refcount_inc(&p->tcfa_refcnt);
308310
if (bind)
309311
atomic_inc(&p->tcfa_bindcnt);
@@ -371,7 +373,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
371373
{
372374
struct tc_action *p = kzalloc(ops->size, GFP_KERNEL);
373375
struct tcf_idrinfo *idrinfo = tn->idrinfo;
374-
struct idr *idr = &idrinfo->action_idr;
375376
int err = -ENOMEM;
376377

377378
if (unlikely(!p))
@@ -389,20 +390,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
389390
goto err2;
390391
}
391392
spin_lock_init(&p->tcfa_lock);
392-
idr_preload(GFP_KERNEL);
393-
spin_lock(&idrinfo->lock);
394-
/* user doesn't specify an index */
395-
if (!index) {
396-
index = 1;
397-
err = idr_alloc_u32(idr, NULL, &index, UINT_MAX, GFP_ATOMIC);
398-
} else {
399-
err = idr_alloc_u32(idr, NULL, &index, index, GFP_ATOMIC);
400-
}
401-
spin_unlock(&idrinfo->lock);
402-
idr_preload_end();
403-
if (err)
404-
goto err3;
405-
406393
p->tcfa_index = index;
407394
p->tcfa_tm.install = jiffies;
408395
p->tcfa_tm.lastuse = jiffies;
@@ -412,16 +399,14 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
412399
&p->tcfa_rate_est,
413400
&p->tcfa_lock, NULL, est);
414401
if (err)
415-
goto err4;
402+
goto err3;
416403
}
417404

418405
p->idrinfo = idrinfo;
419406
p->ops = ops;
420407
INIT_LIST_HEAD(&p->list);
421408
*a = p;
422409
return 0;
423-
err4:
424-
idr_remove(idr, index);
425410
err3:
426411
free_percpu(p->cpu_qstats);
427412
err2:
@@ -437,11 +422,78 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
437422
struct tcf_idrinfo *idrinfo = tn->idrinfo;
438423

439424
spin_lock(&idrinfo->lock);
440-
idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
425+
/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
426+
WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
441427
spin_unlock(&idrinfo->lock);
442428
}
443429
EXPORT_SYMBOL(tcf_idr_insert);
444430

431+
/* Cleanup idr index that was allocated but not initialized. */
432+
433+
void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
434+
{
435+
struct tcf_idrinfo *idrinfo = tn->idrinfo;
436+
437+
spin_lock(&idrinfo->lock);
438+
/* Remove ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
439+
WARN_ON(!IS_ERR(idr_remove(&idrinfo->action_idr, index)));
440+
spin_unlock(&idrinfo->lock);
441+
}
442+
EXPORT_SYMBOL(tcf_idr_cleanup);
443+
444+
/* Check if action with specified index exists. If actions is found, increments
445+
* its reference and bind counters, and return 1. Otherwise insert temporary
446+
* error pointer (to prevent concurrent users from inserting actions with same
447+
* index) and return 0.
448+
*/
449+
450+
int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
451+
struct tc_action **a, int bind)
452+
{
453+
struct tcf_idrinfo *idrinfo = tn->idrinfo;
454+
struct tc_action *p;
455+
int ret;
456+
457+
again:
458+
spin_lock(&idrinfo->lock);
459+
if (*index) {
460+
p = idr_find(&idrinfo->action_idr, *index);
461+
if (IS_ERR(p)) {
462+
/* This means that another process allocated
463+
* index but did not assign the pointer yet.
464+
*/
465+
spin_unlock(&idrinfo->lock);
466+
goto again;
467+
}
468+
469+
if (p) {
470+
refcount_inc(&p->tcfa_refcnt);
471+
if (bind)
472+
atomic_inc(&p->tcfa_bindcnt);
473+
*a = p;
474+
ret = 1;
475+
} else {
476+
*a = NULL;
477+
ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
478+
*index, GFP_ATOMIC);
479+
if (!ret)
480+
idr_replace(&idrinfo->action_idr,
481+
ERR_PTR(-EBUSY), *index);
482+
}
483+
} else {
484+
*index = 1;
485+
*a = NULL;
486+
ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
487+
UINT_MAX, GFP_ATOMIC);
488+
if (!ret)
489+
idr_replace(&idrinfo->action_idr, ERR_PTR(-EBUSY),
490+
*index);
491+
}
492+
spin_unlock(&idrinfo->lock);
493+
return ret;
494+
}
495+
EXPORT_SYMBOL(tcf_idr_check_alloc);
496+
445497
void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
446498
struct tcf_idrinfo *idrinfo)
447499
{

‎net/sched/act_bpf.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,14 +299,17 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
299299

300300
parm = nla_data(tb[TCA_ACT_BPF_PARMS]);
301301

302-
if (!tcf_idr_check(tn, parm->index, act, bind)) {
302+
ret = tcf_idr_check_alloc(tn, &parm->index, act, bind);
303+
if (!ret) {
303304
ret = tcf_idr_create(tn, parm->index, est, act,
304305
&act_bpf_ops, bind, true);
305-
if (ret < 0)
306+
if (ret < 0) {
307+
tcf_idr_cleanup(tn, parm->index);
306308
return ret;
309+
}
307310

308311
res = ACT_P_CREATED;
309-
} else {
312+
} else if (ret > 0) {
310313
/* Don't override defaults. */
311314
if (bind)
312315
return 0;
@@ -315,6 +318,8 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
315318
tcf_idr_release(*act, bind);
316319
return -EEXIST;
317320
}
321+
} else {
322+
return ret;
318323
}
319324

320325
is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];

‎net/sched/act_connmark.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,14 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
118118

119119
parm = nla_data(tb[TCA_CONNMARK_PARMS]);
120120

121-
if (!tcf_idr_check(tn, parm->index, a, bind)) {
121+
ret = tcf_idr_check_alloc(tn, &parm->index, a, bind);
122+
if (!ret) {
122123
ret = tcf_idr_create(tn, parm->index, est, a,
123124
&act_connmark_ops, bind, false);
124-
if (ret)
125+
if (ret) {
126+
tcf_idr_cleanup(tn, parm->index);
125127
return ret;
128+
}
126129

127130
ci = to_connmark(*a);
128131
ci->tcf_action = parm->action;
@@ -131,7 +134,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
131134

132135
tcf_idr_insert(tn, *a);
133136
ret = ACT_P_CREATED;
134-
} else {
137+
} else if (ret > 0) {
135138
ci = to_connmark(*a);
136139
if (bind)
137140
return 0;
@@ -142,6 +145,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
142145
/* replacing action and zone */
143146
ci->tcf_action = parm->action;
144147
ci->zone = parm->zone;
148+
ret = 0;
145149
}
146150

147151
return ret;

‎net/sched/act_csum.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,19 +67,24 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
6767
return -EINVAL;
6868
parm = nla_data(tb[TCA_CSUM_PARMS]);
6969

70-
if (!tcf_idr_check(tn, parm->index, a, bind)) {
70+
err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
71+
if (!err) {
7172
ret = tcf_idr_create(tn, parm->index, est, a,
7273
&act_csum_ops, bind, true);
73-
if (ret)
74+
if (ret) {
75+
tcf_idr_cleanup(tn, parm->index);
7476
return ret;
77+
}
7578
ret = ACT_P_CREATED;
76-
} else {
79+
} else if (err > 0) {
7780
if (bind)/* dont override defaults */
7881
return 0;
7982
if (!ovr) {
8083
tcf_idr_release(*a, bind);
8184
return -EEXIST;
8285
}
86+
} else {
87+
return err;
8388
}
8489

8590
p = to_tcf_csum(*a);

‎net/sched/act_gact.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,19 +91,24 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
9191
}
9292
#endif
9393

94-
if (!tcf_idr_check(tn, parm->index, a, bind)) {
94+
err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
95+
if (!err) {
9596
ret = tcf_idr_create(tn, parm->index, est, a,
9697
&act_gact_ops, bind, true);
97-
if (ret)
98+
if (ret) {
99+
tcf_idr_cleanup(tn, parm->index);
98100
return ret;
101+
}
99102
ret = ACT_P_CREATED;
100-
} else {
103+
} else if (err > 0) {
101104
if (bind)/* dont override defaults */
102105
return 0;
103106
if (!ovr) {
104107
tcf_idr_release(*a, bind);
105108
return -EEXIST;
106109
}
110+
} else {
111+
return err;
107112
}
108113

109114
gact = to_gact(*a);

‎net/sched/act_ife.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
484484
if (!p)
485485
return -ENOMEM;
486486

487-
exists = tcf_idr_check(tn, parm->index, a, bind);
487+
err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
488+
if (err < 0)
489+
return err;
490+
exists = err;
488491
if (exists && bind) {
489492
kfree(p);
490493
return 0;
@@ -494,6 +497,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
494497
ret = tcf_idr_create(tn, parm->index, est, a, &act_ife_ops,
495498
bind, true);
496499
if (ret) {
500+
tcf_idr_cleanup(tn, parm->index);
497501
kfree(p);
498502
return ret;
499503
}

‎net/sched/act_ipt.c‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,28 +119,37 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
119119
if (tb[TCA_IPT_INDEX] != NULL)
120120
index = nla_get_u32(tb[TCA_IPT_INDEX]);
121121

122-
exists = tcf_idr_check(tn, index, a, bind);
122+
err = tcf_idr_check_alloc(tn, &index, a, bind);
123+
if (err < 0)
124+
return err;
125+
exists = err;
123126
if (exists && bind)
124127
return 0;
125128

126129
if (tb[TCA_IPT_HOOK] == NULL || tb[TCA_IPT_TARG] == NULL) {
127130
if (exists)
128131
tcf_idr_release(*a, bind);
132+
else
133+
tcf_idr_cleanup(tn, index);
129134
return -EINVAL;
130135
}
131136

132137
td = (struct xt_entry_target *)nla_data(tb[TCA_IPT_TARG]);
133138
if (nla_len(tb[TCA_IPT_TARG]) < td->u.target_size) {
134139
if (exists)
135140
tcf_idr_release(*a, bind);
141+
else
142+
tcf_idr_cleanup(tn, index);
136143
return -EINVAL;
137144
}
138145

139146
if (!exists) {
140147
ret = tcf_idr_create(tn, index, est, a, ops, bind,
141148
false);
142-
if (ret)
149+
if (ret) {
150+
tcf_idr_cleanup(tn, index);
143151
return ret;
152+
}
144153
ret = ACT_P_CREATED;
145154
} else {
146155
if (bind)/* dont override defaults */

‎net/sched/act_mirred.c‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
7979
struct tcf_mirred *m;
8080
struct net_device *dev;
8181
bool exists = false;
82-
int ret;
82+
int ret, err;
8383

8484
if (!nla) {
8585
NL_SET_ERR_MSG_MOD(extack, "Mirred requires attributes to be passed");
@@ -94,7 +94,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
9494
}
9595
parm = nla_data(tb[TCA_MIRRED_PARMS]);
9696

97-
exists = tcf_idr_check(tn, parm->index, a, bind);
97+
err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
98+
if (err < 0)
99+
return err;
100+
exists = err;
98101
if (exists && bind)
99102
return 0;
100103

@@ -107,6 +110,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
107110
default:
108111
if (exists)
109112
tcf_idr_release(*a, bind);
113+
else
114+
tcf_idr_cleanup(tn, parm->index);
110115
NL_SET_ERR_MSG_MOD(extack, "Unknown mirred option");
111116
return -EINVAL;
112117
}
@@ -115,6 +120,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
115120
if (dev == NULL) {
116121
if (exists)
117122
tcf_idr_release(*a, bind);
123+
else
124+
tcf_idr_cleanup(tn, parm->index);
118125
return -ENODEV;
119126
}
120127
mac_header_xmit = dev_is_mac_header_xmit(dev);
@@ -124,13 +131,16 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
124131

125132
if (!exists) {
126133
if (!dev) {
134+
tcf_idr_cleanup(tn, parm->index);
127135
NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
128136
return -EINVAL;
129137
}
130138
ret = tcf_idr_create(tn, parm->index, est, a,
131139
&act_mirred_ops, bind, true);
132-
if (ret)
140+
if (ret) {
141+
tcf_idr_cleanup(tn, parm->index);
133142
return ret;
143+
}
134144
ret = ACT_P_CREATED;
135145
} else if (!ovr) {
136146
tcf_idr_release(*a, bind);

0 commit comments

Comments
 (0)