| Message ID | 20230321133719.49652-1-nbd@nbd.name (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: ethernet: mtk_eth_soc: fix flow block refcounting | expand |
On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote: > Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call > flow_block_cb_incref unconditionally, even for a newly allocated cb. > Fixes a use-after-free bug. Also fix the accidentally inverted refcount > check on unbind. Hi Felix, my 2c worth. Firstly, it's usually best to have one fix per patch. Second, this seems to conflicts with: [PATCH net-next 1/2] net: ethernet: mtk_eth_soc: add code for offloading flows from wlan devices So I guess that series will need to be rebased on this one at some point. > Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support") > Signed-off-by: Felix Fietkau <nbd@nbd.name> > --- > .../net/ethernet/mediatek/mtk_ppe_offload.c | 23 +++++++++++-------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c > index 81afd5ee3fbf..43cc510b51bd 100644 > --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c > +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c > @@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) > struct mtk_eth *eth = mac->hw; > static LIST_HEAD(block_cb_list); > struct flow_block_cb *block_cb; > + bool register_block = false; > flow_setup_cb_t *cb; > > if (!eth->soc->offload_version) > @@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) > switch (f->command) { > case FLOW_BLOCK_BIND: > block_cb = flow_block_cb_lookup(f->block, cb, dev); > - if (block_cb) { > - flow_block_cb_incref(block_cb); > - return 0; > + if (!block_cb) { > + block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); > + if (IS_ERR(block_cb)) > + return PTR_ERR(block_cb); > + > + register_block = true; > } > - block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); > - if (IS_ERR(block_cb)) > - return PTR_ERR(block_cb); > > - flow_block_cb_add(block_cb, f); > - list_add_tail(&block_cb->driver_list, &block_cb_list); > + flow_block_cb_incref(block_cb); > + > + if (register_block) { > + flow_block_cb_add(block_cb, f); > + list_add_tail(&block_cb->driver_list, &block_cb_list); > + } > return 0; I think the existing code was more idiomatic, and could be maintained by simply adding a call to flow_block_cb_incref() before the call to flow_block_cb_add(). @@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) if (IS_ERR(block_cb)) return PTR_ERR(block_cb); + flow_block_cb_incref(block_cb); flow_block_cb_add(block_cb, f); list_add_tail(&block_cb->driver_list, &block_cb_list); return 0; > case FLOW_BLOCK_UNBIND: > block_cb = flow_block_cb_lookup(f->block, cb, dev); > if (!block_cb) > return -ENOENT; > > - if (flow_block_cb_decref(block_cb)) { > + if (!flow_block_cb_decref(block_cb)) { > flow_block_cb_remove(block_cb, f); > list_del(&block_cb->driver_list); > } > -- > 2.39.0 >
On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote: > On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote: > > Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call > > flow_block_cb_incref unconditionally, even for a newly allocated cb. > > Fixes a use-after-free bug. Also fix the accidentally inverted refcount > > check on unbind. > > Firstly, it's usually best to have one fix per patch. Or at least reword the commit message to make it look like it's fixing the refcounting logic? > > block_cb = flow_block_cb_lookup(f->block, cb, dev); > > - if (block_cb) { > > - flow_block_cb_incref(block_cb); > > - return 0; > > + if (!block_cb) { > > + block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); > > + if (IS_ERR(block_cb)) > > + return PTR_ERR(block_cb); > > + > > + register_block = true; > > } > > - block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); > > - if (IS_ERR(block_cb)) > > - return PTR_ERR(block_cb); > > > > - flow_block_cb_add(block_cb, f); > > - list_add_tail(&block_cb->driver_list, &block_cb_list); > > + flow_block_cb_incref(block_cb); > > + > > + if (register_block) { > > + flow_block_cb_add(block_cb, f); > > + list_add_tail(&block_cb->driver_list, &block_cb_list); > > + } > > return 0; > > I think the existing code was more idiomatic, and could > be maintained by simply adding a call to flow_block_cb_incref() > before the call to flow_block_cb_add(). > > @@ -576,6 +576,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) > if (IS_ERR(block_cb)) > return PTR_ERR(block_cb); > > + flow_block_cb_incref(block_cb); > flow_block_cb_add(block_cb, f); > list_add_tail(&block_cb->driver_list, &block_cb_list); > return 0; That'd be my go to fix as well, FWIW. Alternatively - the block cannot be removed until FLOW_BLOCK_UNBIND is called, right? So the register_block bool can be skipped and refcount taken after flow_block_cb_add() / list_add_tail(). That way at least the new block handling doesn't have to be split into two chunks.
On 23.03.23 06:37, Jakub Kicinski wrote: > On Wed, 22 Mar 2023 15:33:52 +0100 Simon Horman wrote: >> On Tue, Mar 21, 2023 at 02:37:18PM +0100, Felix Fietkau wrote: >> > Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call >> > flow_block_cb_incref unconditionally, even for a newly allocated cb. >> > Fixes a use-after-free bug. Also fix the accidentally inverted refcount >> > check on unbind. >> >> Firstly, it's usually best to have one fix per patch. > > Or at least reword the commit message to make it look like it's fixing > the refcounting logic? I thought that was clear in the title already :) I've considered splitting the patch in two, but decided against it, because it could cause bisect issues. Right now the accidentally inverted logic is preventing the use-after-free bug from showing up with a single flow block, so that would break if I only fix one part without the other. Will send v2 based on your suggestions. - Felix
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c index 81afd5ee3fbf..43cc510b51bd 100644 --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c @@ -554,6 +554,7 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) struct mtk_eth *eth = mac->hw; static LIST_HEAD(block_cb_list); struct flow_block_cb *block_cb; + bool register_block = false; flow_setup_cb_t *cb; if (!eth->soc->offload_version) @@ -568,23 +569,27 @@ mtk_eth_setup_tc_block(struct net_device *dev, struct flow_block_offload *f) switch (f->command) { case FLOW_BLOCK_BIND: block_cb = flow_block_cb_lookup(f->block, cb, dev); - if (block_cb) { - flow_block_cb_incref(block_cb); - return 0; + if (!block_cb) { + block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); + if (IS_ERR(block_cb)) + return PTR_ERR(block_cb); + + register_block = true; } - block_cb = flow_block_cb_alloc(cb, dev, dev, NULL); - if (IS_ERR(block_cb)) - return PTR_ERR(block_cb); - flow_block_cb_add(block_cb, f); - list_add_tail(&block_cb->driver_list, &block_cb_list); + flow_block_cb_incref(block_cb); + + if (register_block) { + flow_block_cb_add(block_cb, f); + list_add_tail(&block_cb->driver_list, &block_cb_list); + } return 0; case FLOW_BLOCK_UNBIND: block_cb = flow_block_cb_lookup(f->block, cb, dev); if (!block_cb) return -ENOENT; - if (flow_block_cb_decref(block_cb)) { + if (!flow_block_cb_decref(block_cb)) { flow_block_cb_remove(block_cb, f); list_del(&block_cb->driver_list); }
Since we call flow_block_cb_decref on FLOW_BLOCK_UNBIND, we need to call flow_block_cb_incref unconditionally, even for a newly allocated cb. Fixes a use-after-free bug. Also fix the accidentally inverted refcount check on unbind. Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support") Signed-off-by: Felix Fietkau <nbd@nbd.name> --- .../net/ethernet/mediatek/mtk_ppe_offload.c | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-)