| Message ID | 20240528103754.98985-1-donald.hunter@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [net-next,v1] netfilter: nfnetlink: convert kfree_skb to consume_skb | expand |
On Tue, May 28, 2024 at 11:37:54AM +0100, Donald Hunter wrote: > Use consume_skb in the batch code path to avoid generating spurious > NOT_SPECIFIED skb drop reasons. > > Signed-off-by: Donald Hunter <donald.hunter@gmail.com> Hi Donald, I do wonder if this is the correct approach. I'm happy to stand corrected, but my understanding is that consume_skb() is for situations where the skb is no longer needed for reasons other than errors. But some of these call-sites do appear to be error paths of sorts. ...
Simon Horman <horms@kernel.org> writes: > On Tue, May 28, 2024 at 11:37:54AM +0100, Donald Hunter wrote: >> Use consume_skb in the batch code path to avoid generating spurious >> NOT_SPECIFIED skb drop reasons. >> >> Signed-off-by: Donald Hunter <donald.hunter@gmail.com> > > Hi Donald, > > I do wonder if this is the correct approach. I'm happy to stand corrected, > but my understanding is that consume_skb() is for situations where the skb > is no longer needed for reasons other than errors. But some of these > call-sites do appear to be error paths of sorts. > > ... Hi Simon, They all look to be application layer errors which are either communicated back to the client or cause a replay. My understanding is that consume_skb() should be used here since kfree_skb() now implies a (transport?) drop.
On Mon, Jun 03, 2024 at 10:19:27AM +0100, Donald Hunter wrote: > Simon Horman <horms@kernel.org> writes: > > > On Tue, May 28, 2024 at 11:37:54AM +0100, Donald Hunter wrote: > >> Use consume_skb in the batch code path to avoid generating spurious > >> NOT_SPECIFIED skb drop reasons. > >> > >> Signed-off-by: Donald Hunter <donald.hunter@gmail.com> > > > > Hi Donald, > > > > I do wonder if this is the correct approach. I'm happy to stand corrected, > > but my understanding is that consume_skb() is for situations where the skb > > is no longer needed for reasons other than errors. But some of these > > call-sites do appear to be error paths of sorts. > > > > ... > > Hi Simon, > > They all look to be application layer errors which are either > communicated back to the client or cause a replay. My understanding is > that consume_skb() should be used here since kfree_skb() now implies a > (transport?) drop. Hi Donald, Thanks, that makes sense to me.
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index 4abf660c7baf..c164abcc326b 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -402,27 +402,27 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, { nfnl_unlock(subsys_id); netlink_ack(oskb, nlh, -EOPNOTSUPP, NULL); - return kfree_skb(skb); + return consume_skb(skb); } } if (!ss->valid_genid || !ss->commit || !ss->abort) { nfnl_unlock(subsys_id); netlink_ack(oskb, nlh, -EOPNOTSUPP, NULL); - return kfree_skb(skb); + return consume_skb(skb); } if (!try_module_get(ss->owner)) { nfnl_unlock(subsys_id); netlink_ack(oskb, nlh, -EOPNOTSUPP, NULL); - return kfree_skb(skb); + return consume_skb(skb); } if (!ss->valid_genid(net, genid)) { module_put(ss->owner); nfnl_unlock(subsys_id); netlink_ack(oskb, nlh, -ERESTART, NULL); - return kfree_skb(skb); + return consume_skb(skb); } nfnl_unlock(subsys_id); @@ -565,7 +565,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, if (status & NFNL_BATCH_REPLAY) { ss->abort(net, oskb, NFNL_ABORT_AUTOLOAD); nfnl_err_reset(&err_list); - kfree_skb(skb); + consume_skb(skb); module_put(ss->owner); goto replay; } else if (status == NFNL_BATCH_DONE) { @@ -590,7 +590,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, err = ss->abort(net, oskb, abort_action); if (err == -EAGAIN) { nfnl_err_reset(&err_list); - kfree_skb(skb); + consume_skb(skb); module_put(ss->owner); status |= NFNL_BATCH_FAILURE; goto replay_abort; @@ -598,7 +598,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh, } nfnl_err_deliver(&err_list, oskb); - kfree_skb(skb); + consume_skb(skb); module_put(ss->owner); }
Use consume_skb in the batch code path to avoid generating spurious NOT_SPECIFIED skb drop reasons. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- net/netfilter/nfnetlink.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)