| Message ID | 20241002155550.15016-1-fw@strlen.de |
|---|---|
| Headers | show |
| Series | netfilter: use skb_drop_reason in more places | expand |
Hi Florian, On Wed, Oct 02, 2024 at 05:55:38PM +0200, Florian Westphal wrote: > Provide more precise drop information rather than doing the freeing > from core.c:nf_hook_slow(). > > First patch is a small preparation patch, rest coverts NF_DROP > locations of NF_DROP_REASON(). One question regarding this series. Most spots still rely on EPERM which is the default reason for NF_DROP. I wonder if it is worth updating all these spots to use NF_DROP_REASON with EPERM. I think patchset becomes smaller if it is only used to provide a better reason than EPERM. Thanks.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > One question regarding this series. > > Most spots still rely on EPERM which is the default reason for > NF_DROP. core converts NF_DROP to EPERM if no errno value is set, correct. > I wonder if it is worth updating all these spots to use NF_DROP_REASON > with EPERM. I think patchset becomes smaller if it is only used to > provide a better reason than EPERM. I'm not following, sorry. What do you mean? This is not about errno. NF_DROP_REASON() calls kfree_skb, so tooling can show location other than nf_hook_slow(). Or do you mean using a different macro that always sets EPERM?
On Sat, Oct 12, 2024 at 04:42:16PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > One question regarding this series. > > > > Most spots still rely on EPERM which is the default reason for > > NF_DROP. > > core converts NF_DROP to EPERM if no errno value is set, correct. > > > I wonder if it is worth updating all these spots to use NF_DROP_REASON > > with EPERM. I think patchset becomes smaller if it is only used to > > provide a better reason than EPERM. > > I'm not following, sorry. What do you mean? > > This is not about errno. NF_DROP_REASON() calls kfree_skb, so tooling > can show location other than nf_hook_slow(). Right. > Or do you mean using a different macro that always sets EPERM? Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is shorter? NF_DROP_REASON(pkt->skb, -EPERM) And add a new macro for br_netfilter NF_BR_DROP_REASON which does not always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for this new macro if you like). Or you think the existing generic long version of NF_DROP_REASON is convenient to have? Thanks
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Or do you mean using a different macro that always sets EPERM? > > Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is > shorter? > > NF_DROP_REASON(pkt->skb, -EPERM) > > And add a new macro for br_netfilter NF_BR_DROP_REASON which does not > always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for > this new macro if you like). NF_DROP_REASON is already in the tree and currently most users use something other than SKB_DROP_REASON_NETFILTER_DROP. I did not yet add new enum values or a dedicated nf namespace (enum skb_drop_reason_subsys), because I did not see a reason and wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat, whatever). If you like, I can add NF_FREE_SKB(skb, errno) and rework this set to use that?
On Sat, Oct 12, 2024 at 05:54:48PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > Or do you mean using a different macro that always sets EPERM? > > > > Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is > > shorter? > > > > NF_DROP_REASON(pkt->skb, -EPERM) > > > > And add a new macro for br_netfilter NF_BR_DROP_REASON which does not > > always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for > > this new macro if you like). > > NF_DROP_REASON is already in the tree and currently most users use > something other than SKB_DROP_REASON_NETFILTER_DROP. > > I did not yet add new enum values or a dedicated nf namespace > (enum skb_drop_reason_subsys), because I did not see a reason and > wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat, > whatever). Does this mean values exposed through tracing infrastructure can change or these are part of uapi? From what I read from you, I understand it is possible to change SKB_DROP_REASON_NETFILTER_DROP to a more specific sub-subsystem tag in the future without issues. > If you like, I can add NF_FREE_SKB(skb, errno) and rework this > set to use that? Not strong about this. I was exploring if it should be possible to remove (repetitive) information in the code that can be assumed to be implicit, I still like the word "REASON" in the macro for grepping. I think we can just move on with this series as-is if you prefer and add new macros incrementally to refine.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > I did not yet add new enum values or a dedicated nf namespace > > (enum skb_drop_reason_subsys), because I did not see a reason and > > wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat, > > whatever). > > Does this mean values exposed through tracing infrastructure can > change or these are part of uapi? The enum has had values added (not just appended), so its not considered uapi. > From what I read from you, I > understand it is possible to change SKB_DROP_REASON_NETFILTER_DROP to > a more specific sub-subsystem tag in the future without issues. Thats correct. > > If you like, I can add NF_FREE_SKB(skb, errno) and rework this > > set to use that? > > Not strong about this. I was exploring if it should be possible to > remove (repetitive) information in the code that can be assumed to be > implicit, I still like the word "REASON" in the macro for grepping. Yes, common name is good. > I think we can just move on with this series as-is if you prefer and > add new macros incrementally to refine. I think we can refine later if there are use-cases for that.