Re: [Patch net-next] net_sched: fix a potential out-of-bound access

[Vlad Buslov <vladbu@mellanox.com>]

On Thu 09 Aug 2018 at 21:43, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Aug 9, 2018 at 12:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Before version V5 of my action API patchset this functionality was
>> implemented in exactly the same way as in your patch. Unfortunately, it
>> has a double-free bug. The problem is that if you have multiple
>> actions(N) being deleted, and deleted succeeded for first K actions,
>> this implementation will try to delete all N actions second time
>> (including first K actions that were already deleted). That is why I
>> added 'acts_deleted' variable that tracks actual amount of actions that
>> were deleted successfully, and only delete last N-K actions in case of
>> error.
>
> Interesting, I didn't notice you call it for tcf_del_notify()'s failure too.
>
> But this is easy to resolve, we can just set succeeded ones to NULL
> and teach tcf_action_put_many() to scan the whole array but
> skip NULL's.

Both approaches have their own tradeoffs. I considered just skipping
NULLs, but in my experience such approach might introduce hard to debug
bugs when all callers are expected to have either valid pointers or
NULLs in whole array. Then someone forgets to zero unused elements, but
it continues to work because stack memory just happens to be 0. Until
some code path uses same stack space before and leaves some junk there
instead of convenient zeroes...

Also, it becomes more computationally expensive to always scan whole
array for non-NULL pointers, instead of exiting when first NULL is
encountered. I don't think it is a major concert for current
implementation with MAX_PRIO==32, just a general preference of mine.

>
>
>>
>> In order to fix that issue I did following code changes in V5:
>> - Added 'acts_deleted' variable to delete only actions [K, N) in case of
>> error.
>> - Extended 'actions' array size by one to ensure that it always ends
>> with NULL pointer.
>
> Oh, I see, this is not how we use C, you can at least rollback
> by passing acts_deleted as a parameter as the start of the array.
> You picked the most confusing way to handle it.

Noted. I will refrain from passing pointers to arbitrary array elements
as beginning-of-array from now on.

>
> I will send an updated patch.