Skip to content

Commit e33d2b7

Browse files
congwangdavem330
authored andcommitted
idr: fix overflow case for idr_for_each_entry_ul()
idr_for_each_entry_ul() is buggy as it can't handle overflow case correctly. When we have an ID == UINT_MAX, it becomes an infinite loop. This happens when running on 32-bit CPU where unsigned long has the same size with unsigned int. There is no better way to fix this than casting it to a larger integer, but we can't just 64 bit integer on 32 bit CPU. Instead we could just use an additional integer to help us to detect this overflow case, that is, adding a new parameter to this macro. Fortunately tc action is its only user right now. Fixes: 65a206c ("net/sched: Change act_api and act_xxx modules to use IDR") Reported-by: Li Shuang <[email protected]> Tested-by: Davide Caratti <[email protected]> Cc: Matthew Wilcox <[email protected]> Cc: Chris Mi <[email protected]> Signed-off-by: Cong Wang <[email protected]> Signed-off-by: David S. Miller <[email protected]>
1 parent eb1f5c0 commit e33d2b7

File tree

2 files changed

+11
-5
lines changed

2 files changed

+11
-5
lines changed

include/linux/idr.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,14 +191,17 @@ static inline void idr_preload_end(void)
191191
* idr_for_each_entry_ul() - Iterate over an IDR's elements of a given type.
192192
* @idr: IDR handle.
193193
* @entry: The type * to use as cursor.
194+
* @tmp: A temporary placeholder for ID.
194195
* @id: Entry ID.
195196
*
196197
* @entry and @id do not need to be initialized before the loop, and
197198
* after normal termination @entry is left with the value NULL. This
198199
* is convenient for a "not found" value.
199200
*/
200-
#define idr_for_each_entry_ul(idr, entry, id) \
201-
for (id = 0; ((entry) = idr_get_next_ul(idr, &(id))) != NULL; ++id)
201+
#define idr_for_each_entry_ul(idr, entry, tmp, id) \
202+
for (tmp = 0, id = 0; \
203+
tmp <= id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
204+
tmp = id, ++id)
202205

203206
/**
204207
* idr_for_each_entry_continue() - Continue iteration over an IDR's elements of a given type

net/sched/act_api.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,13 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
221221
struct idr *idr = &idrinfo->action_idr;
222222
struct tc_action *p;
223223
unsigned long id = 1;
224+
unsigned long tmp;
224225

225226
mutex_lock(&idrinfo->lock);
226227

227228
s_i = cb->args[0];
228229

229-
idr_for_each_entry_ul(idr, p, id) {
230+
idr_for_each_entry_ul(idr, p, tmp, id) {
230231
index++;
231232
if (index < s_i)
232233
continue;
@@ -292,6 +293,7 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
292293
struct idr *idr = &idrinfo->action_idr;
293294
struct tc_action *p;
294295
unsigned long id = 1;
296+
unsigned long tmp;
295297

296298
nest = nla_nest_start_noflag(skb, 0);
297299
if (nest == NULL)
@@ -300,7 +302,7 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
300302
goto nla_put_failure;
301303

302304
mutex_lock(&idrinfo->lock);
303-
idr_for_each_entry_ul(idr, p, id) {
305+
idr_for_each_entry_ul(idr, p, tmp, id) {
304306
ret = tcf_idr_release_unsafe(p);
305307
if (ret == ACT_P_DELETED) {
306308
module_put(ops->owner);
@@ -533,8 +535,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
533535
struct tc_action *p;
534536
int ret;
535537
unsigned long id = 1;
538+
unsigned long tmp;
536539

537-
idr_for_each_entry_ul(idr, p, id) {
540+
idr_for_each_entry_ul(idr, p, tmp, id) {
538541
ret = __tcf_idr_release(p, false, true);
539542
if (ret == ACT_P_DELETED)
540543
module_put(ops->owner);

0 commit comments

Comments
 (0)