mlxsw: core: Introduce flexible actions support
Hello Jiri Pirko,
The patch 4cda7d8d7098: "mlxsw: core: Introduce flexible actions support" from Feb 3, 2017, leads to the following static checker warning:
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c:387 mlxsw_afa_block_commit()
error: dereferencing freed memory 'set'
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
355 int mlxsw_afa_block_commit(struct mlxsw_afa_block *block) 356 {
357 struct mlxsw_afa_set *set = block->cur_set;
358 struct mlxsw_afa_set *prev_set;
359 int err;
360
361 block->cur_set = NULL;
362
363 /* Go over all linked sets starting from last
364 * and try to find existing set in the hash table.
365 * In case it is not there, assign a KVD linear index
366 * and insert it.
367 */
368 do {
369 prev_set = set->prev;
370 set = mlxsw_afa_set_get(block->afa, set);
371 if (IS_ERR(set)) {
372 err = PTR_ERR(set);
373 goto rollback;
374 }
375 if (prev_set) {
376 prev_set->next = set;
377 mlxsw_afa_set_next_set(prev_set, set->kvdl_index);
378 set = prev_set;
379 }
380 } while (prev_set);
381
382 block->first_set = set;
383 block->finished = true;
384 return 0; 385
386 rollback:
387 while ((set = set->next))
388 mlxsw_afa_set_put(block->afa, set);
"set" is refcounted. The heuristic I'm using assumes that if it's refcounted with an atomic type then ignore the possibility that mlxsw_afa_set_put() will free "set". This works very well generally and this is only the second time I've seen it fail because we're using regular types for refcounting.
It's possible that we know that we're holding multiple references to "set" so it will never be freed, but I normally don't feel bad sending false positive warnings if the code is very recent so here we are. :)
389 return err;
390 }
regards,
dan carpenter
Quoted 20 lines · Dan Carpenter
Hello Jiri Pirko,
The patch
4cda7d8d7098: "mlxsw: core: Introduce flexible actions support" from Feb 3, 2017, leads to the following static checker warning:
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c:387mlxsw_afa_block_commit()
error: dereferencing freed memory 'set'
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
355 intmlxsw_afa_block_commit(struct mlxsw_afa_block *block)356 { 357 struct mlxsw_afa_set *set = block->cur_set; 358 struct mlxsw_afa_set *prev_set; 359 int err; 360 361 block->cur_set = NULL; 362 363 /* Go over all linked sets starting from last 364 * and try to find existing set in the hash table.
Quoted 8 lines
365 * In case it is not there, assign a KVD linear index
366 * and insert it.
367 */
368 do {
369 prev_set = set->prev;
370 set = mlxsw_afa_set_get(block->afa, set);
371 if (IS_ERR(set)) {
372 err = PTR_ERR(set);Oh... Also it can be an error pointer on this path.
Quoted 7 lines
373 goto rollback;
374 }
375 if (prev_set) {
376 prev_set->next = set;
377 mlxsw_afa_set_next_set(prev_set, set->kvdl_index);
378 set = prev_set;
379 }Quoted 8 lines
380 } while (prev_set); 381 382 block->first_set = set; 383 block->finished = true; 384 return 0; 385 386 rollback: 387 while ((set = set->next))
^^^^^^^^^^^^^^^
That means this dereference will oops.
regards,
dan carpenter
Quoted 17 lines
388 mlxsw_afa_set_put(block->afa, set);"set" is refcounted. The heuristic I'm using assumes that if it's refcounted with an atomic type then ignore the possibility that
mlxsw_afa_set_put()will free "set". This works very well generally and this is only the second time I've seen it fail because we're using regular types for refcounting.It's possible that we know that we're holding multiple references to "set" so it will never be freed, but I normally don't feel bad sending false positive warnings if the code is very recent so here we are. :)
389 return err; 390 }regards,
dan carpenter
Quoted 47 lines · Tue, Feb 07, 2017 at 11:33:31AM CET, [email protected] wrote:
Quoted 20 lines · Dan Carpenter
Hello Jiri Pirko,
The patch
4cda7d8d7098: "mlxsw: core: Introduce flexible actions support" from Feb 3, 2017, leads to the following static checker warning:
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c:387mlxsw_afa_block_commit()
error: dereferencing freed memory 'set'
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_actions.c
355 intmlxsw_afa_block_commit(struct mlxsw_afa_block *block)356 { 357 struct mlxsw_afa_set *set = block->cur_set; 358 struct mlxsw_afa_set *prev_set; 359 int err; 360 361 block->cur_set = NULL; 362 363 /* Go over all linked sets starting from last 364 * and try to find existing set in the hash table.Quoted 8 lines
365 * In case it is not there, assign a KVD linear index 366 * and insert it. 367 */ 368 do { 369 prev_set = set->prev; 370 set = mlxsw_afa_set_get(block->afa, set); 371 if (IS_ERR(set)) { 372 err = PTR_ERR(set);Oh... Also it can be an error pointer on this path.
Quoted 7 lines
373 goto rollback; 374 } 375 if (prev_set) { 376 prev_set->next = set; 377 mlxsw_afa_set_next_set(prev_set, set->kvdl_index); 378 set = prev_set; 379 }Quoted 8 lines
380 } while (prev_set); 381 382 block->first_set = set; 383 block->finished = true; 384 return 0; 385 386 rollback: 387 while ((set = set->next))
Quoted 3 lines
^^^^^^^^^^^^^^^That means this dereference will oops.
Will fix this. Thanks.
Quoted 22 lines
regards,
dan carpenterQuoted 17 lines
388 mlxsw_afa_set_put(block->afa, set);"set" is refcounted. The heuristic I'm using assumes that if it's refcounted with an atomic type then ignore the possibility that
mlxsw_afa_set_put()will free "set". This works very well generally and this is only the second time I've seen it fail because we're using regular types for refcounting.It's possible that we know that we're holding multiple references to "set" so it will never be freed, but I normally don't feel bad sending false positive warnings if the code is very recent so here we are. :)
389 return err; 390 }regards,
dan carpenter