netdev - Re: [PATCH net 2/2] net/sched: flower: fix error handler on replace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 26 Apr 2023 17:39:09 +0200
From:   Ivan Vecera <ivecera@...hat.com>
To:     Vlad Buslov <vladbu@...dia.com>,
        Pedro Tammela <pctammela@...atatu.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        marcelo.leitner@...il.com, paulb@...dia.com,
        simon.horman@...igine.com
Subject: Re: [PATCH net 2/2] net/sched: flower: fix error handler on replace



On 26. 04. 23 16:46, Vlad Buslov wrote:
> On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@...atatu.com> wrote:
>> On 26/04/2023 09:14, Vlad Buslov wrote:
>>> When replacing a filter (i.e. 'fold' pointer is not NULL) the insertion of
>>> new filter to idr is postponed until later in code since handle is already
>>> provided by the user. However, the error handling code in fl_change()
>>> always assumes that the new filter had been inserted into idr. If error
>>> handler is reached when replacing existing filter it may remove it from idr
>>> therefore making it unreachable for delete or dump afterwards. Fix the
>>> issue by verifying that 'fold' argument wasn't provided by caller before
>>> calling idr_remove().
>>> Fixes: 08a0063df3ae ("net/sched: flower: Move filter handle initialization
>>> earlier")
>>> Signed-off-by: Vlad Buslov <vladbu@...dia.com>
>>> ---
>>>    net/sched/cls_flower.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>>> index 1844545bef37..a1c4ee2e0be2 100644
>>> --- a/net/sched/cls_flower.c
>>> +++ b/net/sched/cls_flower.c
>>> @@ -2339,7 +2339,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
>>>    errout_mask:
>>>    	fl_mask_put(head, fnew->mask);
>>>    errout_idr:
>>> -	idr_remove(&head->handle_idr, fnew->handle);
>>> +	if (!fold)
>>> +		idr_remove(&head->handle_idr, fnew->handle);
>>>    	__fl_put(fnew);
>>>    errout_tb:
>>>    	kfree(tb);
>>
>> Actually this seems to be fixing the same issue:
>> https://lore.kernel.org/all/20230425140604.169881-1-ivecera@redhat.com/
> 
> Indeed it does, I've missed that patch. However, it seems there
> is an issue with Ivan's approach. Consider what would happen when
> fold!=NULL && in_ht==false and rhashtable_insert_fast() fails here:
> 
> 
>          if (fold) {
>                  /* Fold filter was deleted concurrently. Retry lookup. */
>                  if (fold->deleted) {
>                          err = -EAGAIN;
>                          goto errout_hw;
>                  }
> 
>                  fnew->handle = handle; // <-- fnew->handle is assigned
> 
>                  if (!in_ht) {
>                          struct rhashtable_params params =
>                                  fnew->mask->filter_ht_params;
> 
>                          err = rhashtable_insert_fast(&fnew->mask->ht,
>                                                       &fnew->ht_node,
>                                                       params);
>                          if (err)
>                                  goto errout_hw; /* <-- err is set, go to
>                                                       error handler here */
>                          in_ht = true;
>                  }
> 
>                  refcount_inc(&fnew->refcnt);
>                  rhashtable_remove_fast(&fold->mask->ht,
>                                         &fold->ht_node,
>                                         fold->mask->filter_ht_params);
>                  /* !!! we never get to insert fnew into idr here, if ht insertion fails */
>                  idr_replace(&head->handle_idr, fnew, fnew->handle);
>                  list_replace_rcu(&fold->list, &fnew->list);
>                  fold->deleted = true;
> 
>                  spin_unlock(&tp->lock);
> 
>                  fl_mask_put(head, fold->mask);
>                  if (!tc_skip_hw(fold->flags))
>                          fl_hw_destroy_filter(tp, fold, rtnl_held, NULL);
>                  tcf_unbind_filter(tp, &fold->res);
>                  /* Caller holds reference to fold, so refcnt is always > 0
>                   * after this.
>                   */
>                  refcount_dec(&fold->refcnt);
>                  __fl_put(fold);
>          }
> 
> ...
> 
>   errout_ht:
>           spin_lock(&tp->lock);
>   errout_hw:
>           fnew->deleted = true;
>           spin_unlock(&tp->lock);
>           if (!tc_skip_hw(fnew->flags))
>                   fl_hw_destroy_filter(tp, fnew, rtnl_held, NULL);
>           if (in_ht)
>                   rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node,
>                                          fnew->mask->filter_ht_params);
>   errout_mask:
>           fl_mask_put(head, fnew->mask);
>   errout_idr:
>           /* !!! On next line we remove handle that we don't actually own */
>           idr_remove(&head->handle_idr, fnew->handle);
>           __fl_put(fnew);
>   errout_tb:
>           kfree(tb);
>   errout_mask_alloc:
>           tcf_queue_work(&mask->rwork, fl_uninit_mask_free_work);
>   errout_fold:
>           if (fold)
>                   __fl_put(fold);
>           return err;
> 
> 
> Also, if I understood the idea behind Ivan's fix correctly, it relies on
> the fact that calling idr_remove() with handle==0 is a noop. I prefer my
> approach slightly better as it is more explicit IMO.
> 
> Thoughts?

Yes, your approach is better...

Acked-by: Ivan Vecera <ivecera@...hat.com>