* [bug report] mlxsw: core: Introduce flexible actions support
@ 2017-02-07 7:18 Dan Carpenter
2017-02-07 10:33 ` Dan Carpenter
2017-02-07 10:43 ` Jiri Pirko
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2017-02-07 7:18 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] mlxsw: core: Introduce flexible actions support
2017-02-07 7:18 [bug report] mlxsw: core: Introduce flexible actions support Dan Carpenter
@ 2017-02-07 10:33 ` Dan Carpenter
2017-02-07 10:43 ` Jiri Pirko
1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2017-02-07 10:33 UTC (permalink / raw)
To: kernel-janitors
On Tue, Feb 07, 2017 at 10:18:06AM +0300, Dan Carpenter wrote:
> 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);
Oh... Also it can be an error pointer on this path.
> 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))
^^^^^^^^^^^^^^^
That means this dereference will oops.
regards,
dan carpenter
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] mlxsw: core: Introduce flexible actions support
2017-02-07 7:18 [bug report] mlxsw: core: Introduce flexible actions support Dan Carpenter
2017-02-07 10:33 ` Dan Carpenter
@ 2017-02-07 10:43 ` Jiri Pirko
1 sibling, 0 replies; 3+ messages in thread
From: Jiri Pirko @ 2017-02-07 10:43 UTC (permalink / raw)
To: kernel-janitors
Tue, Feb 07, 2017 at 11:33:31AM CET, dan.carpenter@oracle.com wrote:
>On Tue, Feb 07, 2017 at 10:18:06AM +0300, Dan Carpenter wrote:
>> 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);
>
>Oh... Also it can be an error pointer on this path.
>
>> 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))
> ^^^^^^^^^^^^^^^
>
>That means this dereference will oops.
Will fix this. Thanks.
>
>regards,
>dan carpenter
>
>
>> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-02-07 10:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-07 7:18 [bug report] mlxsw: core: Introduce flexible actions support Dan Carpenter
2017-02-07 10:33 ` Dan Carpenter
2017-02-07 10:43 ` Jiri Pirko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox