* [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
@ 2021-04-08 7:47 Jianbo Liu
2021-04-08 21:16 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jianbo Liu @ 2021-04-08 7:47 UTC (permalink / raw)
To: netdev; +Cc: pablo, Jianbo Liu, Roi Dayan
When device is removed, indirect block is unregisterd. As
bo->unlocked_driver_cb is not initialized, the following UBSAN is
triggered.
UBSAN: invalid-load in net/sched/cls_api.c:1496:10
load of value 6 is not a valid value for type '_Bool'
This patch fixes the warning by calling device's indr block bind
callback, and unlocked_driver_cb is assigned with correct value.
Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
include/net/flow_offload.h | 11 ++++++-----
net/core/flow_offload.c | 13 +++++++------
net/sched/cls_api.c | 11 ++++++++---
3 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index dc5c1e69cd9f..8cdc60833890 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -459,6 +459,11 @@ typedef int flow_setup_cb_t(enum tc_setup_type type, void *type_data,
struct flow_block_cb;
+typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
+ enum tc_setup_type type, void *type_data,
+ void *data,
+ void (*cleanup)(struct flow_block_cb *block_cb));
+
struct flow_block_indr {
struct list_head list;
struct net_device *dev;
@@ -466,6 +471,7 @@ struct flow_block_indr {
enum flow_block_binder_type binder_type;
void *data;
void *cb_priv;
+ flow_indr_block_bind_cb_t *setup_cb;
void (*cleanup)(struct flow_block_cb *block_cb);
};
@@ -562,11 +568,6 @@ static inline void flow_block_init(struct flow_block *flow_block)
INIT_LIST_HEAD(&flow_block->cb_list);
}
-typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
- enum tc_setup_type type, void *type_data,
- void *data,
- void (*cleanup)(struct flow_block_cb *block_cb));
-
int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv);
void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
void (*release)(void *cb_priv));
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 715b67f6c62f..85a3d8530952 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -373,7 +373,8 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
}
EXPORT_SYMBOL(flow_indr_dev_register);
-static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
+static void __flow_block_indr_cleanup(struct flow_indr_dev *indr_dev,
+ void (*release)(void *cb_priv),
void *cb_priv,
struct list_head *cleanup_list)
{
@@ -381,8 +382,10 @@ static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
list_for_each_entry_safe(this, next, &flow_block_indr_list, indr.list) {
if (this->release == release &&
- this->indr.cb_priv == cb_priv)
+ this->indr.cb_priv == cb_priv) {
+ this->indr.setup_cb = indr_dev->cb;
list_move(&this->indr.list, cleanup_list);
+ }
}
}
@@ -390,10 +393,8 @@ static void flow_block_indr_notify(struct list_head *cleanup_list)
{
struct flow_block_cb *this, *next;
- list_for_each_entry_safe(this, next, cleanup_list, indr.list) {
- list_del(&this->indr.list);
+ list_for_each_entry_safe(this, next, cleanup_list, indr.list)
this->indr.cleanup(this);
- }
}
void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
@@ -418,7 +419,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
return;
}
- __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
+ __flow_block_indr_cleanup(this, release, cb_priv, &cleanup_list);
mutex_unlock(&flow_indr_block_lock);
flow_block_indr_notify(&cleanup_list);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d3db70865d66..b213206da728 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -646,7 +646,7 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
struct net_device *dev = block_cb->indr.dev;
struct Qdisc *sch = block_cb->indr.sch;
struct netlink_ext_ack extack = {};
- struct flow_block_offload bo;
+ struct flow_block_offload bo = {};
tcf_block_offload_init(&bo, dev, sch, FLOW_BLOCK_UNBIND,
block_cb->indr.binder_type,
@@ -654,8 +654,13 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
&extack);
rtnl_lock();
down_write(&block->cb_lock);
- list_del(&block_cb->driver_list);
- list_move(&block_cb->list, &bo.cb_list);
+ if (!block_cb->indr.setup_cb ||
+ block_cb->indr.setup_cb(dev, sch, block_cb->indr.cb_priv,
+ TC_SETUP_BLOCK, &bo, block, NULL)) {
+ list_del(&block_cb->indr.list);
+ list_del(&block_cb->driver_list);
+ list_move(&block_cb->list, &bo.cb_list);
+ }
tcf_block_unbind(block, &bo);
up_write(&block->cb_lock);
rtnl_unlock();
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
2021-04-08 7:47 [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind Jianbo Liu
@ 2021-04-08 21:16 ` Jakub Kicinski
2021-04-09 6:25 ` Jianbo Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2021-04-08 21:16 UTC (permalink / raw)
To: Jianbo Liu; +Cc: netdev, pablo, Roi Dayan
On Thu, 8 Apr 2021 07:47:18 +0000 Jianbo Liu wrote:
> When device is removed, indirect block is unregisterd. As
> bo->unlocked_driver_cb is not initialized, the following UBSAN is
> triggered.
>
> UBSAN: invalid-load in net/sched/cls_api.c:1496:10
> load of value 6 is not a valid value for type '_Bool'
>
> This patch fixes the warning by calling device's indr block bind
> callback, and unlocked_driver_cb is assigned with correct value.
>
> Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
It's been a while since I looked at this code but I don't understand
what you're doing here.
The init in tc_block_indr_cleanup() makes sense. What's the change to
setup_cb achieving? Thanks.
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index dc5c1e69cd9f..8cdc60833890 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -459,6 +459,11 @@ typedef int flow_setup_cb_t(enum tc_setup_type type, void *type_data,
>
> struct flow_block_cb;
>
> +typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
> + enum tc_setup_type type, void *type_data,
> + void *data,
> + void (*cleanup)(struct flow_block_cb *block_cb));
> +
> struct flow_block_indr {
> struct list_head list;
> struct net_device *dev;
> @@ -466,6 +471,7 @@ struct flow_block_indr {
> enum flow_block_binder_type binder_type;
> void *data;
> void *cb_priv;
> + flow_indr_block_bind_cb_t *setup_cb;
> void (*cleanup)(struct flow_block_cb *block_cb);
> };
>
> @@ -562,11 +568,6 @@ static inline void flow_block_init(struct flow_block *flow_block)
> INIT_LIST_HEAD(&flow_block->cb_list);
> }
>
> -typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
> - enum tc_setup_type type, void *type_data,
> - void *data,
> - void (*cleanup)(struct flow_block_cb *block_cb));
> -
> int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv);
> void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> void (*release)(void *cb_priv));
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 715b67f6c62f..85a3d8530952 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c
> @@ -373,7 +373,8 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
> }
> EXPORT_SYMBOL(flow_indr_dev_register);
>
> -static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
> +static void __flow_block_indr_cleanup(struct flow_indr_dev *indr_dev,
> + void (*release)(void *cb_priv),
> void *cb_priv,
> struct list_head *cleanup_list)
> {
> @@ -381,8 +382,10 @@ static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
>
> list_for_each_entry_safe(this, next, &flow_block_indr_list, indr.list) {
> if (this->release == release &&
> - this->indr.cb_priv == cb_priv)
> + this->indr.cb_priv == cb_priv) {
> + this->indr.setup_cb = indr_dev->cb;
> list_move(&this->indr.list, cleanup_list);
> + }
> }
> }
>
> @@ -390,10 +393,8 @@ static void flow_block_indr_notify(struct list_head *cleanup_list)
> {
> struct flow_block_cb *this, *next;
>
> - list_for_each_entry_safe(this, next, cleanup_list, indr.list) {
> - list_del(&this->indr.list);
> + list_for_each_entry_safe(this, next, cleanup_list, indr.list)
> this->indr.cleanup(this);
> - }
> }
>
> void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> @@ -418,7 +419,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> return;
> }
>
> - __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
> + __flow_block_indr_cleanup(this, release, cb_priv, &cleanup_list);
> mutex_unlock(&flow_indr_block_lock);
>
> flow_block_indr_notify(&cleanup_list);
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d3db70865d66..b213206da728 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -646,7 +646,7 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
> struct net_device *dev = block_cb->indr.dev;
> struct Qdisc *sch = block_cb->indr.sch;
> struct netlink_ext_ack extack = {};
> - struct flow_block_offload bo;
> + struct flow_block_offload bo = {};
>
> tcf_block_offload_init(&bo, dev, sch, FLOW_BLOCK_UNBIND,
> block_cb->indr.binder_type,
> @@ -654,8 +654,13 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
> &extack);
> rtnl_lock();
> down_write(&block->cb_lock);
> - list_del(&block_cb->driver_list);
> - list_move(&block_cb->list, &bo.cb_list);
> + if (!block_cb->indr.setup_cb ||
> + block_cb->indr.setup_cb(dev, sch, block_cb->indr.cb_priv,
> + TC_SETUP_BLOCK, &bo, block, NULL)) {
> + list_del(&block_cb->indr.list);
> + list_del(&block_cb->driver_list);
> + list_move(&block_cb->list, &bo.cb_list);
> + }
> tcf_block_unbind(block, &bo);
> up_write(&block->cb_lock);
> rtnl_unlock();
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
2021-04-08 21:16 ` Jakub Kicinski
@ 2021-04-09 6:25 ` Jianbo Liu
2021-04-09 16:01 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jianbo Liu @ 2021-04-09 6:25 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, pablo, Roi Dayan
The 04/08/2021 14:16, Jakub Kicinski wrote:
> On Thu, 8 Apr 2021 07:47:18 +0000 Jianbo Liu wrote:
> > When device is removed, indirect block is unregisterd. As
> > bo->unlocked_driver_cb is not initialized, the following UBSAN is
> > triggered.
> >
> > UBSAN: invalid-load in net/sched/cls_api.c:1496:10
> > load of value 6 is not a valid value for type '_Bool'
> >
> > This patch fixes the warning by calling device's indr block bind
> > callback, and unlocked_driver_cb is assigned with correct value.
> >
> > Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> > Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> > Reviewed-by: Roi Dayan <roid@nvidia.com>
>
> It's been a while since I looked at this code but I don't understand
> what you're doing here.
To fix the UBSAN warning in tcf_block_unbind. It's easily triggered when
netdev is removed before tunnel netdev.
>
> The init in tc_block_indr_cleanup() makes sense. What's the change to
> setup_cb achieving? Thanks.
But unlocked_driver_cb of flow_block_offload is not initialized in init.
Calling setup_cb is to get the correct value from driver.
>
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index dc5c1e69cd9f..8cdc60833890 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -459,6 +459,11 @@ typedef int flow_setup_cb_t(enum tc_setup_type type, void *type_data,
> >
> > struct flow_block_cb;
> >
> > +typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
> > + enum tc_setup_type type, void *type_data,
> > + void *data,
> > + void (*cleanup)(struct flow_block_cb *block_cb));
> > +
> > struct flow_block_indr {
> > struct list_head list;
> > struct net_device *dev;
> > @@ -466,6 +471,7 @@ struct flow_block_indr {
> > enum flow_block_binder_type binder_type;
> > void *data;
> > void *cb_priv;
> > + flow_indr_block_bind_cb_t *setup_cb;
> > void (*cleanup)(struct flow_block_cb *block_cb);
> > };
> >
> > @@ -562,11 +568,6 @@ static inline void flow_block_init(struct flow_block *flow_block)
> > INIT_LIST_HEAD(&flow_block->cb_list);
> > }
> >
> > -typedef int flow_indr_block_bind_cb_t(struct net_device *dev, struct Qdisc *sch, void *cb_priv,
> > - enum tc_setup_type type, void *type_data,
> > - void *data,
> > - void (*cleanup)(struct flow_block_cb *block_cb));
> > -
> > int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv);
> > void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> > void (*release)(void *cb_priv));
> > diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> > index 715b67f6c62f..85a3d8530952 100644
> > --- a/net/core/flow_offload.c
> > +++ b/net/core/flow_offload.c
> > @@ -373,7 +373,8 @@ int flow_indr_dev_register(flow_indr_block_bind_cb_t *cb, void *cb_priv)
> > }
> > EXPORT_SYMBOL(flow_indr_dev_register);
> >
> > -static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
> > +static void __flow_block_indr_cleanup(struct flow_indr_dev *indr_dev,
> > + void (*release)(void *cb_priv),
> > void *cb_priv,
> > struct list_head *cleanup_list)
> > {
> > @@ -381,8 +382,10 @@ static void __flow_block_indr_cleanup(void (*release)(void *cb_priv),
> >
> > list_for_each_entry_safe(this, next, &flow_block_indr_list, indr.list) {
> > if (this->release == release &&
> > - this->indr.cb_priv == cb_priv)
> > + this->indr.cb_priv == cb_priv) {
> > + this->indr.setup_cb = indr_dev->cb;
> > list_move(&this->indr.list, cleanup_list);
> > + }
> > }
> > }
> >
> > @@ -390,10 +393,8 @@ static void flow_block_indr_notify(struct list_head *cleanup_list)
> > {
> > struct flow_block_cb *this, *next;
> >
> > - list_for_each_entry_safe(this, next, cleanup_list, indr.list) {
> > - list_del(&this->indr.list);
> > + list_for_each_entry_safe(this, next, cleanup_list, indr.list)
> > this->indr.cleanup(this);
> > - }
> > }
> >
> > void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> > @@ -418,7 +419,7 @@ void flow_indr_dev_unregister(flow_indr_block_bind_cb_t *cb, void *cb_priv,
> > return;
> > }
> >
> > - __flow_block_indr_cleanup(release, cb_priv, &cleanup_list);
> > + __flow_block_indr_cleanup(this, release, cb_priv, &cleanup_list);
> > mutex_unlock(&flow_indr_block_lock);
> >
> > flow_block_indr_notify(&cleanup_list);
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index d3db70865d66..b213206da728 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -646,7 +646,7 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
> > struct net_device *dev = block_cb->indr.dev;
> > struct Qdisc *sch = block_cb->indr.sch;
> > struct netlink_ext_ack extack = {};
> > - struct flow_block_offload bo;
> > + struct flow_block_offload bo = {};
> >
> > tcf_block_offload_init(&bo, dev, sch, FLOW_BLOCK_UNBIND,
> > block_cb->indr.binder_type,
> > @@ -654,8 +654,13 @@ static void tc_block_indr_cleanup(struct flow_block_cb *block_cb)
> > &extack);
> > rtnl_lock();
> > down_write(&block->cb_lock);
> > - list_del(&block_cb->driver_list);
> > - list_move(&block_cb->list, &bo.cb_list);
> > + if (!block_cb->indr.setup_cb ||
> > + block_cb->indr.setup_cb(dev, sch, block_cb->indr.cb_priv,
> > + TC_SETUP_BLOCK, &bo, block, NULL)) {
> > + list_del(&block_cb->indr.list);
> > + list_del(&block_cb->driver_list);
> > + list_move(&block_cb->list, &bo.cb_list);
> > + }
> > tcf_block_unbind(block, &bo);
> > up_write(&block->cb_lock);
> > rtnl_unlock();
>
--
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
2021-04-09 6:25 ` Jianbo Liu
@ 2021-04-09 16:01 ` Jakub Kicinski
2021-04-12 1:21 ` Jianbo Liu
2021-04-20 8:11 ` Jianbo Liu
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-04-09 16:01 UTC (permalink / raw)
To: Jianbo Liu; +Cc: netdev, pablo, Roi Dayan
On Fri, 9 Apr 2021 06:25:56 +0000 Jianbo Liu wrote:
> The 04/08/2021 14:16, Jakub Kicinski wrote:
> > On Thu, 8 Apr 2021 07:47:18 +0000 Jianbo Liu wrote:
> > > When device is removed, indirect block is unregisterd. As
> > > bo->unlocked_driver_cb is not initialized, the following UBSAN is
> > > triggered.
> > >
> > > UBSAN: invalid-load in net/sched/cls_api.c:1496:10
> > > load of value 6 is not a valid value for type '_Bool'
> > >
> > > This patch fixes the warning by calling device's indr block bind
> > > callback, and unlocked_driver_cb is assigned with correct value.
> > >
> > > Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> > > Reviewed-by: Roi Dayan <roid@nvidia.com>
> >
> > It's been a while since I looked at this code but I don't understand
> > what you're doing here.
>
> To fix the UBSAN warning in tcf_block_unbind. It's easily triggered when
> netdev is removed before tunnel netdev.
>
> > The init in tc_block_indr_cleanup() makes sense. What's the change to
> > setup_cb achieving? Thanks.
>
> But unlocked_driver_cb of flow_block_offload is not initialized in init.
> Calling setup_cb is to get the correct value from driver.
I'm trying to understand what became of this code :/ Was there no call
with FLOW_BLOCK_UNBIND to the driver when driver was unregistering
before your change?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
2021-04-09 16:01 ` Jakub Kicinski
@ 2021-04-12 1:21 ` Jianbo Liu
2021-04-20 8:11 ` Jianbo Liu
1 sibling, 0 replies; 6+ messages in thread
From: Jianbo Liu @ 2021-04-12 1:21 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, pablo, Roi Dayan
The 04/09/2021 09:01, Jakub Kicinski wrote:
> On Fri, 9 Apr 2021 06:25:56 +0000 Jianbo Liu wrote:
> > The 04/08/2021 14:16, Jakub Kicinski wrote:
> > > On Thu, 8 Apr 2021 07:47:18 +0000 Jianbo Liu wrote:
> > > > When device is removed, indirect block is unregisterd. As
> > > > bo->unlocked_driver_cb is not initialized, the following UBSAN is
> > > > triggered.
> > > >
> > > > UBSAN: invalid-load in net/sched/cls_api.c:1496:10
> > > > load of value 6 is not a valid value for type '_Bool'
> > > >
> > > > This patch fixes the warning by calling device's indr block bind
> > > > callback, and unlocked_driver_cb is assigned with correct value.
> > > >
> > > > Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> > > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> > > > Reviewed-by: Roi Dayan <roid@nvidia.com>
> > >
> > > It's been a while since I looked at this code but I don't understand
> > > what you're doing here.
> >
> > To fix the UBSAN warning in tcf_block_unbind. It's easily triggered when
> > netdev is removed before tunnel netdev.
> >
> > > The init in tc_block_indr_cleanup() makes sense. What's the change to
> > > setup_cb achieving? Thanks.
> >
> > But unlocked_driver_cb of flow_block_offload is not initialized in init.
> > Calling setup_cb is to get the correct value from driver.
>
> I'm trying to understand what became of this code :/ Was there no call
> with FLOW_BLOCK_UNBIND to the driver when driver was unregistering
> before your change?
Yes, I think so.
>
--
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind
2021-04-09 16:01 ` Jakub Kicinski
2021-04-12 1:21 ` Jianbo Liu
@ 2021-04-20 8:11 ` Jianbo Liu
1 sibling, 0 replies; 6+ messages in thread
From: Jianbo Liu @ 2021-04-20 8:11 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, pablo, Roi Dayan
The 04/09/2021 09:01, Jakub Kicinski wrote:
> On Fri, 9 Apr 2021 06:25:56 +0000 Jianbo Liu wrote:
> > The 04/08/2021 14:16, Jakub Kicinski wrote:
> > > On Thu, 8 Apr 2021 07:47:18 +0000 Jianbo Liu wrote:
> > > > When device is removed, indirect block is unregisterd. As
> > > > bo->unlocked_driver_cb is not initialized, the following UBSAN is
> > > > triggered.
> > > >
> > > > UBSAN: invalid-load in net/sched/cls_api.c:1496:10
> > > > load of value 6 is not a valid value for type '_Bool'
> > > >
> > > > This patch fixes the warning by calling device's indr block bind
> > > > callback, and unlocked_driver_cb is assigned with correct value.
> > > >
> > > > Fixes: 0fdcf78d5973 ("net: use flow_indr_dev_setup_offload()")
> > > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> > > > Reviewed-by: Roi Dayan <roid@nvidia.com>
> > >
> > > It's been a while since I looked at this code but I don't understand
> > > what you're doing here.
> >
> > To fix the UBSAN warning in tcf_block_unbind. It's easily triggered when
> > netdev is removed before tunnel netdev.
> >
> > > The init in tc_block_indr_cleanup() makes sense. What's the change to
> > > setup_cb achieving? Thanks.
> >
> > But unlocked_driver_cb of flow_block_offload is not initialized in init.
> > Calling setup_cb is to get the correct value from driver.
>
> I'm trying to understand what became of this code :/ Was there no call
> with FLOW_BLOCK_UNBIND to the driver when driver was unregistering
> before your change?
>
Gentle ping.
Should I need any change for this patch?
Thanks!
--
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-20 8:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 7:47 [PATCH net] net: flow_offload: Fix UBSAN invalid-load warning in tcf_block_unbind Jianbo Liu
2021-04-08 21:16 ` Jakub Kicinski
2021-04-09 6:25 ` Jianbo Liu
2021-04-09 16:01 ` Jakub Kicinski
2021-04-12 1:21 ` Jianbo Liu
2021-04-20 8:11 ` Jianbo Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).