| Message ID | 20230426121415.2149732-3-vladbu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes for miss to tc action series | expand |
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@nvidia.com> LGTM Reviewed-by: Pedro Tammela <pctammela@mojatatu.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);
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@nvidia.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/
On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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?
On 26/04/2023 11:46, Vlad Buslov wrote: > On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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? I agree with your analysis
On 26. 04. 23 16:46, Vlad Buslov wrote: > On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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@redhat.com>
On 26/04/2023 15: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@nvidia.com> Reviewed-by: Paul Blakey <paulb@nvidia.com>
On Wed, Apr 26, 2023 at 05:39:09PM +0200, Ivan Vecera wrote: > > > On 26. 04. 23 16:46, Vlad Buslov wrote: > > On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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@redhat.com> In the meantime it seems that Ivan's patch has been accepted into net. - [net] net/sched: flower: Fix wrong handle assignment during filter change https://git.kernel.org/netdev/net/c/32eff6bacec2 Is some adjustment to this patch required to take that into account? >
On 28. 04. 23 9:11, Simon Horman wrote: > On Wed, Apr 26, 2023 at 05:39:09PM +0200, Ivan Vecera wrote: >> >> >> On 26. 04. 23 16:46, Vlad Buslov wrote: >>> On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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@redhat.com> > > In the meantime it seems that Ivan's patch has been accepted into net. > > - [net] net/sched: flower: Fix wrong handle assignment during filter change > https://git.kernel.org/netdev/net/c/32eff6bacec2 > > Is some adjustment to this patch required to take that into account? I think something like this is necessary to cover Vlad's findings: diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 6ab6aadc07b8da..ce937baefcf00e 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -2279,8 +2279,6 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, goto errout_hw; } - fnew->handle = handle; - if (!in_ht) { struct rhashtable_params params = fnew->mask->filter_ht_params; @@ -2297,6 +2295,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, rhashtable_remove_fast(&fold->mask->ht, &fold->ht_node, fold->mask->filter_ht_params); + fnew->handle = handle; idr_replace(&head->handle_idr, fnew, fnew->handle); list_replace_rcu(&fold->list, &fnew->list); fold->deleted = true; Just move fnew->handle assignment immediately prior idr_replace(). Thoughts? Ivan
On Fri 28 Apr 2023 at 10:20, Ivan Vecera <ivecera@redhat.com> wrote: > On 28. 04. 23 9:11, Simon Horman wrote: >> On Wed, Apr 26, 2023 at 05:39:09PM +0200, Ivan Vecera wrote: >>> >>> >>> On 26. 04. 23 16:46, Vlad Buslov wrote: >>>> On Wed 26 Apr 2023 at 11:22, Pedro Tammela <pctammela@mojatatu.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@nvidia.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@redhat.com> >> In the meantime it seems that Ivan's patch has been accepted into net. >> - [net] net/sched: flower: Fix wrong handle assignment during filter change >> https://git.kernel.org/netdev/net/c/32eff6bacec2 >> Is some adjustment to this patch required to take that into account? > > I think something like this is necessary to cover Vlad's findings: > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index 6ab6aadc07b8da..ce937baefcf00e 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -2279,8 +2279,6 @@ static int fl_change(struct net *net, struct sk_buff > *in_skb, > goto errout_hw; > } > > - fnew->handle = handle; > - > if (!in_ht) { > struct rhashtable_params params = > fnew->mask->filter_ht_params; > @@ -2297,6 +2295,7 @@ static int fl_change(struct net *net, struct sk_buff > *in_skb, > rhashtable_remove_fast(&fold->mask->ht, > &fold->ht_node, > fold->mask->filter_ht_params); > + fnew->handle = handle; > idr_replace(&head->handle_idr, fnew, fnew->handle); > list_replace_rcu(&fold->list, &fnew->list); > fold->deleted = true; > > Just move fnew->handle assignment immediately prior idr_replace(). > > Thoughts? Note that with these changes (both accepted patch and preceding diff) you are exposing filter to dapapath access (datapath looks up filter via hash table, not idr) with its handle set to 0 initially and then resent while already accessible. After taking a quick look at Paul's miss-to-action code it seems that handle value used by datapath is taken from struct tcf_exts_miss_cookie_node not from filter directly, so such approach likely doesn't break anything existing, but I might have missed something.
On Fri, 28 Apr 2023 14:03:19 +0300 Vlad Buslov wrote: > Note that with these changes (both accepted patch and preceding diff) > you are exposing filter to dapapath access (datapath looks up filter via > hash table, not idr) with its handle set to 0 initially and then resent > while already accessible. After taking a quick look at Paul's > miss-to-action code it seems that handle value used by datapath is taken > from struct tcf_exts_miss_cookie_node not from filter directly, so such > approach likely doesn't break anything existing, but I might have missed > something. Did we deadlock in this discussion, or the issue was otherwise fixed?
On Tue 02 May 2023 at 19:44, Jakub Kicinski <kuba@kernel.org> wrote: > On Fri, 28 Apr 2023 14:03:19 +0300 Vlad Buslov wrote: >> Note that with these changes (both accepted patch and preceding diff) >> you are exposing filter to dapapath access (datapath looks up filter via >> hash table, not idr) with its handle set to 0 initially and then resent >> while already accessible. After taking a quick look at Paul's >> miss-to-action code it seems that handle value used by datapath is taken >> from struct tcf_exts_miss_cookie_node not from filter directly, so such >> approach likely doesn't break anything existing, but I might have missed >> something. > > Did we deadlock in this discussion, or the issue was otherwise fixed? From my side I explained why in my opinion Ivan's fix doesn't cover all cases and my approach is better overall. Don't know what else to discuss since it seems that everyone agreed.
On Thu, 2023-05-04 at 16:40 +0300, Vlad Buslov wrote: > On Tue 02 May 2023 at 19:44, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 28 Apr 2023 14:03:19 +0300 Vlad Buslov wrote: > > > Note that with these changes (both accepted patch and preceding diff) > > > you are exposing filter to dapapath access (datapath looks up filter via > > > hash table, not idr) with its handle set to 0 initially and then resent > > > while already accessible. After taking a quick look at Paul's > > > miss-to-action code it seems that handle value used by datapath is taken > > > from struct tcf_exts_miss_cookie_node not from filter directly, so such > > > approach likely doesn't break anything existing, but I might have missed > > > something. > > > > Did we deadlock in this discussion, or the issue was otherwise fixed? > > From my side I explained why in my opinion Ivan's fix doesn't cover all > cases and my approach is better overall. Don't know what else to discuss > since it seems that everyone agreed. Do I read correctly that we need a revert of Ivan's patch to safely apply this series? If so, could you please repost including such revert? Thanks. Paolo
On Thu 04 May 2023 at 16:24, Paolo Abeni <pabeni@redhat.com> wrote: > On Thu, 2023-05-04 at 16:40 +0300, Vlad Buslov wrote: >> On Tue 02 May 2023 at 19:44, Jakub Kicinski <kuba@kernel.org> wrote: >> > On Fri, 28 Apr 2023 14:03:19 +0300 Vlad Buslov wrote: >> > > Note that with these changes (both accepted patch and preceding diff) >> > > you are exposing filter to dapapath access (datapath looks up filter via >> > > hash table, not idr) with its handle set to 0 initially and then resent >> > > while already accessible. After taking a quick look at Paul's >> > > miss-to-action code it seems that handle value used by datapath is taken >> > > from struct tcf_exts_miss_cookie_node not from filter directly, so such >> > > approach likely doesn't break anything existing, but I might have missed >> > > something. >> > >> > Did we deadlock in this discussion, or the issue was otherwise fixed? >> >> From my side I explained why in my opinion Ivan's fix doesn't cover all >> cases and my approach is better overall. Don't know what else to discuss >> since it seems that everyone agreed. > > Do I read correctly that we need a revert of Ivan's patch to safely > apply this series? If so, could you please repost including such > revert? I don't believe our fixes conflict, it is just that Ivan's should become redundant with mine applied. Anyway, I've just sent V2 with added revert.
On Thu, May 04, 2023 at 09:32:40PM +0300, Vlad Buslov wrote: > > On Thu 04 May 2023 at 16:24, Paolo Abeni <pabeni@redhat.com> wrote: > > On Thu, 2023-05-04 at 16:40 +0300, Vlad Buslov wrote: > >> On Tue 02 May 2023 at 19:44, Jakub Kicinski <kuba@kernel.org> wrote: > >> > On Fri, 28 Apr 2023 14:03:19 +0300 Vlad Buslov wrote: > >> > > Note that with these changes (both accepted patch and preceding diff) > >> > > you are exposing filter to dapapath access (datapath looks up filter via > >> > > hash table, not idr) with its handle set to 0 initially and then resent > >> > > while already accessible. After taking a quick look at Paul's > >> > > miss-to-action code it seems that handle value used by datapath is taken > >> > > from struct tcf_exts_miss_cookie_node not from filter directly, so such > >> > > approach likely doesn't break anything existing, but I might have missed > >> > > something. > >> > > >> > Did we deadlock in this discussion, or the issue was otherwise fixed? > >> > >> From my side I explained why in my opinion Ivan's fix doesn't cover all > >> cases and my approach is better overall. Don't know what else to discuss > >> since it seems that everyone agreed. > > > > Do I read correctly that we need a revert of Ivan's patch to safely > > apply this series? If so, could you please repost including such > > revert? > > I don't believe our fixes conflict, it is just that Ivan's should become > redundant with mine applied. Anyway, I've just sent V2 with added > revert. Thanks. FWIIW, this matches my understanding of the situation.
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);
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@nvidia.com> --- net/sched/cls_flower.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)