Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/11/23 22:46, Aaron Conole wrote: Eric Garver writes: On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote: On 7/8/23 00:06, Jakub Kicinski wrote: On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: That already exists, right? Johannes added it in the last release for WiFi. I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly to that on a surface. However, looking closer, any value that can be passed into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is kind of defined in net/mac80211/drop.h, unless I'm missing something (very possible, because I don't really know wifi code). The difference, I guess, is that for openvswitch values will be provided by the userpsace application via netlink interface. It'll be just a number not defined anywhere in the kernel. Only the subsystem itself will be defined in order to occupy the range. Garbage in, same garbage out, from the kernel's perspective. To be clear, I think, not defining them in this particular case is better. Definition of every reason that userspace can come up with will add extra uAPI maintenance cost/issues with no practical benefits. Values are not going to be used for anything outside reporting a drop reason and subsystem offset is not part of uAPI anyway. Ah, I see. No, please don't stuff user space defined values into the drop reason. The reasons are for debugging the kernel stack itself. IOW it'd be abuse not reuse. Makes sense. I wasn't sure that's a good solution from a kernel perspective either. It's better than defining all these reasons, IMO, but it's not good enough to be considered acceptable, I agree. How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? One for an explicit drop action with a zero argument and one for an explicit drop with non-zero argument. The exact reason for the error can be retrieved by other means, i.e by looking at the datapath flow dump or OVS logs/traces. This way we can give a user who is catching packet drop traces a signal that there was something wrong with an OVS flow and they can look up exact details from the userspace / flow dump. The point being, most of the flows will have a zero as a drop action argument, i.e. a regular explicit packet drop. It will be hard to figure out which flow exactly we're hitting without looking at the full flow dump. And if the value is non-zero, then it should be immediately obvious which flow is to blame from the dump, as we should not have a lot of such flows. This would still allow us to avoid a maintenance burden of defining every case, which are fairly meaningless for the kernel itself, while having 99% of the information we may need. Jakub, do you think this will be acceptable? Eric, Adrian, Aaron, do you see any problems with such implementation? I see no problems. I'm content with this approach. +1 Sounds like a good plan. Thanks. P.S. There is a plan to add more drop reasons for other places in openvswitch module to catch more regular types of drops like memory issues or upcall failures. So, the drop reason subsystem can be extended later. The explicit drop action is a bit of an odd case here. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Eric Garver writes: > On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote: >> On 7/8/23 00:06, Jakub Kicinski wrote: >> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: >> That already exists, right? Johannes added it in the last release for >> WiFi. >> >>> >> >>> I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves >> >>> similarly >> >>> to that on a surface. However, looking closer, any value that can be >> >>> passed >> >>> into ieee80211_rx_handlers_result() and ends up in the >> >>> kfree_skb_reason() is >> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something >> >>> (very >> >>> possible, because I don't really know wifi code). >> >>> >> >>> The difference, I guess, is that for openvswitch values will be provided >> >>> by >> >>> the userpsace application via netlink interface. It'll be just a number >> >>> not >> >>> defined anywhere in the kernel. Only the subsystem itself will be >> >>> defined >> >>> in order to occupy the range. Garbage in, same garbage out, from the >> >>> kernel's >> >>> perspective. >> >> >> >> To be clear, I think, not defining them in this particular case is better. >> >> Definition of every reason that userspace can come up with will add extra >> >> uAPI maintenance cost/issues with no practical benefits. Values are not >> >> going to be used for anything outside reporting a drop reason and >> >> subsystem >> >> offset is not part of uAPI anyway. >> > >> > Ah, I see. No, please don't stuff user space defined values into >> > the drop reason. The reasons are for debugging the kernel stack >> > itself. IOW it'd be abuse not reuse. >> >> Makes sense. I wasn't sure that's a good solution from a kernel perspective >> either. It's better than defining all these reasons, IMO, but it's not good >> enough to be considered acceptable, I agree. >> >> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and >> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? >> One for an explicit drop action with a zero argument and one for an explicit >> drop with non-zero argument. >> >> The exact reason for the error can be retrieved by other means, i.e by >> looking >> at the datapath flow dump or OVS logs/traces. >> >> This way we can give a user who is catching packet drop traces a signal that >> there was something wrong with an OVS flow and they can look up exact details >> from the userspace / flow dump. >> >> The point being, most of the flows will have a zero as a drop action >> argument, >> i.e. a regular explicit packet drop. It will be hard to figure out which >> flow >> exactly we're hitting without looking at the full flow dump. And if the >> value >> is non-zero, then it should be immediately obvious which flow is to blame >> from >> the dump, as we should not have a lot of such flows. >> >> This would still allow us to avoid a maintenance burden of defining every >> case, >> which are fairly meaningless for the kernel itself, while having 99% of the >> information we may need. >> >> Jakub, do you think this will be acceptable? >> >> Eric, Adrian, Aaron, do you see any problems with such implementation? > > I see no problems. I'm content with this approach. +1 >> P.S. There is a plan to add more drop reasons for other places in openvswitch >> module to catch more regular types of drops like memory issues or upcall >> failures. So, the drop reason subsystem can be extended later. >> The explicit drop action is a bit of an odd case here. >> >> Best regards, Ilya Maximets. >> ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Mon, 10 Jul 2023 20:39:11 +0200 Ilya Maximets wrote: > > As far as I understand what you're proposing, yes :) > > OK. Just to spell it all out: > > Userspace will install a flow with an OVS_FLOW_CMD_NEW: > > match:ip,tcp,... actions:something,something,drop(0) > match:ip,udp,... actions:something,something,drop(42) > > drop() here represents the OVS_ACTION_ATTR_DROP. > > Then, in net/openvswitch/actions.c:do_execute_actions(), while executing > these actions: > > case OVS_ACTION_ATTR_DROP: > kfree_skb_reason(skb, nla_get_u32(a) ? OVS_DROP_ACTION_WITH_ERROR >: OVS_DROP_ACTION); > > Users can enable traces and catch the OVS_DROP_ACTION_WITH_ERROR. > Later they can dump flows with OVS_FLOW_CMD_GET and see that the > error value was 42. nod > >> Eric, Adrian, Aaron, do you see any problems with such implementation? > >> > >> P.S. There is a plan to add more drop reasons for other places in > >> openvswitch > >> module to catch more regular types of drops like memory issues or > >> upcall > >> failures. So, the drop reason subsystem can be extended later. > >> The explicit drop action is a bit of an odd case here. > > > > If you have more than ~4 OvS specific reasons, I wonder if it still > > makes sense to create a reason group/subsystem for OvS (a'la WiFi)? > > I believe, we will easily have more than 4 OVS-specific reasons. A few > from the top of my head: > - upcall failure (failed to send a packet to userspace) > - reached the limit for deferred actions > - reached the recursion limit > > So, creation of a reason group/subsystem seems reasonable to me. SG. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/10/23 19:01, Jakub Kicinski wrote: > On Mon, 10 Jul 2023 18:51:19 +0200 Ilya Maximets wrote: >> Makes sense. I wasn't sure that's a good solution from a kernel perspective >> either. It's better than defining all these reasons, IMO, but it's not good >> enough to be considered acceptable, I agree. >> >> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and >> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? >> One for an explicit drop action with a zero argument and one for an explicit >> drop with non-zero argument. >> >> The exact reason for the error can be retrieved by other means, i.e by >> looking >> at the datapath flow dump or OVS logs/traces. >> >> This way we can give a user who is catching packet drop traces a signal that >> there was something wrong with an OVS flow and they can look up exact details >> from the userspace / flow dump. >> >> The point being, most of the flows will have a zero as a drop action >> argument, >> i.e. a regular explicit packet drop. It will be hard to figure out which >> flow >> exactly we're hitting without looking at the full flow dump. And if the >> value >> is non-zero, then it should be immediately obvious which flow is to blame >> from >> the dump, as we should not have a lot of such flows. >> >> This would still allow us to avoid a maintenance burden of defining every >> case, >> which are fairly meaningless for the kernel itself, while having 99% of the >> information we may need. >> >> Jakub, do you think this will be acceptable? > > As far as I understand what you're proposing, yes :) OK. Just to spell it all out: Userspace will install a flow with an OVS_FLOW_CMD_NEW: match:ip,tcp,... actions:something,something,drop(0) match:ip,udp,... actions:something,something,drop(42) drop() here represents the OVS_ACTION_ATTR_DROP. Then, in net/openvswitch/actions.c:do_execute_actions(), while executing these actions: case OVS_ACTION_ATTR_DROP: kfree_skb_reason(skb, nla_get_u32(a) ? OVS_DROP_ACTION_WITH_ERROR : OVS_DROP_ACTION); Users can enable traces and catch the OVS_DROP_ACTION_WITH_ERROR. Later they can dump flows with OVS_FLOW_CMD_GET and see that the error value was 42. > >> Eric, Adrian, Aaron, do you see any problems with such implementation? >> >> P.S. There is a plan to add more drop reasons for other places in openvswitch >> module to catch more regular types of drops like memory issues or upcall >> failures. So, the drop reason subsystem can be extended later. >> The explicit drop action is a bit of an odd case here. > > If you have more than ~4 OvS specific reasons, I wonder if it still > makes sense to create a reason group/subsystem for OvS (a'la WiFi)? I believe, we will easily have more than 4 OVS-specific reasons. A few from the top of my head: - upcall failure (failed to send a packet to userspace) - reached the limit for deferred actions - reached the recursion limit So, creation of a reason group/subsystem seems reasonable to me. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote: > On 7/8/23 00:06, Jakub Kicinski wrote: > > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: > That already exists, right? Johannes added it in the last release for > WiFi. > >>> > >>> I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves > >>> similarly > >>> to that on a surface. However, looking closer, any value that can be > >>> passed > >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() > >>> is > >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very > >>> possible, because I don't really know wifi code). > >>> > >>> The difference, I guess, is that for openvswitch values will be provided > >>> by > >>> the userpsace application via netlink interface. It'll be just a number > >>> not > >>> defined anywhere in the kernel. Only the subsystem itself will be defined > >>> in order to occupy the range. Garbage in, same garbage out, from the > >>> kernel's > >>> perspective. > >> > >> To be clear, I think, not defining them in this particular case is better. > >> Definition of every reason that userspace can come up with will add extra > >> uAPI maintenance cost/issues with no practical benefits. Values are not > >> going to be used for anything outside reporting a drop reason and subsystem > >> offset is not part of uAPI anyway. > > > > Ah, I see. No, please don't stuff user space defined values into > > the drop reason. The reasons are for debugging the kernel stack > > itself. IOW it'd be abuse not reuse. > > Makes sense. I wasn't sure that's a good solution from a kernel perspective > either. It's better than defining all these reasons, IMO, but it's not good > enough to be considered acceptable, I agree. > > How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and > OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? > One for an explicit drop action with a zero argument and one for an explicit > drop with non-zero argument. > > The exact reason for the error can be retrieved by other means, i.e by looking > at the datapath flow dump or OVS logs/traces. > > This way we can give a user who is catching packet drop traces a signal that > there was something wrong with an OVS flow and they can look up exact details > from the userspace / flow dump. > > The point being, most of the flows will have a zero as a drop action argument, > i.e. a regular explicit packet drop. It will be hard to figure out which flow > exactly we're hitting without looking at the full flow dump. And if the value > is non-zero, then it should be immediately obvious which flow is to blame from > the dump, as we should not have a lot of such flows. > > This would still allow us to avoid a maintenance burden of defining every > case, > which are fairly meaningless for the kernel itself, while having 99% of the > information we may need. > > Jakub, do you think this will be acceptable? > > Eric, Adrian, Aaron, do you see any problems with such implementation? I see no problems. I'm content with this approach. > P.S. There is a plan to add more drop reasons for other places in openvswitch > module to catch more regular types of drops like memory issues or upcall > failures. So, the drop reason subsystem can be extended later. > The explicit drop action is a bit of an odd case here. > > Best regards, Ilya Maximets. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Mon, 10 Jul 2023 18:51:19 +0200 Ilya Maximets wrote: > Makes sense. I wasn't sure that's a good solution from a kernel perspective > either. It's better than defining all these reasons, IMO, but it's not good > enough to be considered acceptable, I agree. > > How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and > OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? > One for an explicit drop action with a zero argument and one for an explicit > drop with non-zero argument. > > The exact reason for the error can be retrieved by other means, i.e by looking > at the datapath flow dump or OVS logs/traces. > > This way we can give a user who is catching packet drop traces a signal that > there was something wrong with an OVS flow and they can look up exact details > from the userspace / flow dump. > > The point being, most of the flows will have a zero as a drop action argument, > i.e. a regular explicit packet drop. It will be hard to figure out which flow > exactly we're hitting without looking at the full flow dump. And if the value > is non-zero, then it should be immediately obvious which flow is to blame from > the dump, as we should not have a lot of such flows. > > This would still allow us to avoid a maintenance burden of defining every > case, > which are fairly meaningless for the kernel itself, while having 99% of the > information we may need. > > Jakub, do you think this will be acceptable? As far as I understand what you're proposing, yes :) > Eric, Adrian, Aaron, do you see any problems with such implementation? > > P.S. There is a plan to add more drop reasons for other places in openvswitch > module to catch more regular types of drops like memory issues or upcall > failures. So, the drop reason subsystem can be extended later. > The explicit drop action is a bit of an odd case here. If you have more than ~4 OvS specific reasons, I wonder if it still makes sense to create a reason group/subsystem for OvS (a'la WiFi)? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/8/23 00:06, Jakub Kicinski wrote: > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: That already exists, right? Johannes added it in the last release for WiFi. >>> >>> I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves >>> similarly >>> to that on a surface. However, looking closer, any value that can be passed >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very >>> possible, because I don't really know wifi code). >>> >>> The difference, I guess, is that for openvswitch values will be provided by >>> the userpsace application via netlink interface. It'll be just a number not >>> defined anywhere in the kernel. Only the subsystem itself will be defined >>> in order to occupy the range. Garbage in, same garbage out, from the >>> kernel's >>> perspective. >> >> To be clear, I think, not defining them in this particular case is better. >> Definition of every reason that userspace can come up with will add extra >> uAPI maintenance cost/issues with no practical benefits. Values are not >> going to be used for anything outside reporting a drop reason and subsystem >> offset is not part of uAPI anyway. > > Ah, I see. No, please don't stuff user space defined values into > the drop reason. The reasons are for debugging the kernel stack > itself. IOW it'd be abuse not reuse. Makes sense. I wasn't sure that's a good solution from a kernel perspective either. It's better than defining all these reasons, IMO, but it's not good enough to be considered acceptable, I agree. How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? One for an explicit drop action with a zero argument and one for an explicit drop with non-zero argument. The exact reason for the error can be retrieved by other means, i.e by looking at the datapath flow dump or OVS logs/traces. This way we can give a user who is catching packet drop traces a signal that there was something wrong with an OVS flow and they can look up exact details from the userspace / flow dump. The point being, most of the flows will have a zero as a drop action argument, i.e. a regular explicit packet drop. It will be hard to figure out which flow exactly we're hitting without looking at the full flow dump. And if the value is non-zero, then it should be immediately obvious which flow is to blame from the dump, as we should not have a lot of such flows. This would still allow us to avoid a maintenance burden of defining every case, which are fairly meaningless for the kernel itself, while having 99% of the information we may need. Jakub, do you think this will be acceptable? Eric, Adrian, Aaron, do you see any problems with such implementation? P.S. There is a plan to add more drop reasons for other places in openvswitch module to catch more regular types of drops like memory issues or upcall failures. So, the drop reason subsystem can be extended later. The explicit drop action is a bit of an odd case here. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: > >> That already exists, right? Johannes added it in the last release for > >> WiFi. > > > > I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves > > similarly > > to that on a surface. However, looking closer, any value that can be passed > > into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is > > kind of defined in net/mac80211/drop.h, unless I'm missing something (very > > possible, because I don't really know wifi code). > > > > The difference, I guess, is that for openvswitch values will be provided by > > the userpsace application via netlink interface. It'll be just a number not > > defined anywhere in the kernel. Only the subsystem itself will be defined > > in order to occupy the range. Garbage in, same garbage out, from the > > kernel's > > perspective. > > To be clear, I think, not defining them in this particular case is better. > Definition of every reason that userspace can come up with will add extra > uAPI maintenance cost/issues with no practical benefits. Values are not > going to be used for anything outside reporting a drop reason and subsystem > offset is not part of uAPI anyway. Ah, I see. No, please don't stuff user space defined values into the drop reason. The reasons are for debugging the kernel stack itself. IOW it'd be abuse not reuse. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/7/23 17:29, Ilya Maximets wrote: > On 7/7/23 17:00, Jakub Kicinski wrote: >> On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote: >>> A wild idea: How about we do not define actual reasons? i.e. define a >>> subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where >>> 'value' is whatever userspace gives as long as it is within a subsystem >>> range? >> >> That already exists, right? Johannes added it in the last release for WiFi. > > I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly > to that on a surface. However, looking closer, any value that can be passed > into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is > kind of defined in net/mac80211/drop.h, unless I'm missing something (very > possible, because I don't really know wifi code). > > The difference, I guess, is that for openvswitch values will be provided by > the userpsace application via netlink interface. It'll be just a number not > defined anywhere in the kernel. Only the subsystem itself will be defined > in order to occupy the range. Garbage in, same garbage out, from the kernel's > perspective. To be clear, I think, not defining them in this particular case is better. Definition of every reason that userspace can come up with will add extra uAPI maintenance cost/issues with no practical benefits. Values are not going to be used for anything outside reporting a drop reason and subsystem offset is not part of uAPI anyway. > > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/7/23 17:00, Jakub Kicinski wrote: > On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote: >> A wild idea: How about we do not define actual reasons? i.e. define a >> subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where >> 'value' is whatever userspace gives as long as it is within a subsystem >> range? > > That already exists, right? Johannes added it in the last release for WiFi. I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly to that on a surface. However, looking closer, any value that can be passed into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is kind of defined in net/mac80211/drop.h, unless I'm missing something (very possible, because I don't really know wifi code). The difference, I guess, is that for openvswitch values will be provided by the userpsace application via netlink interface. It'll be just a number not defined anywhere in the kernel. Only the subsystem itself will be defined in order to occupy the range. Garbage in, same garbage out, from the kernel's perspective. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Fri, 7 Jul 2023 12:30:38 +0200 Ilya Maximets wrote: > A wild idea: How about we do not define actual reasons? i.e. define a > subsystem and just call kfree_skb_reason(skb, SUBSYSTEM | value), where > 'value' is whatever userspace gives as long as it is within a subsystem > range? That already exists, right? Johannes added it in the last release for WiFi. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On 7/6/23 15:57, Eric Garver wrote:
> On Thu, Jul 06, 2023 at 08:54:16AM -0400, Aaron Conole wrote:
>> Eric Garver writes:
>>
>>> This adds an explicit drop action. This is used by OVS to drop packets
>>> for which it cannot determine what to do. An explicit action in the
>>> kernel allows passing the reason _why_ the packet is being dropped. We
>>> can then use perf tracing to match on the drop reason.
>>>
>>> e.g. trace all OVS dropped skbs
>>>
>>> # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
>>> [..]
>>> 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
>>> location:0xc0d9b462, protocol: 2048, reason: 196610)
>>>
>>> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>>>
>>> Signed-off-by: Eric Garver
>>> ---
>>> include/uapi/linux/openvswitch.h| 2 ++
>>> net/openvswitch/actions.c | 13 +
>>> net/openvswitch/flow_netlink.c | 12 +++-
>>> .../testing/selftests/net/openvswitch/ovs-dpctl.py | 3 +++
>>> 4 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/openvswitch.h
>>> b/include/uapi/linux/openvswitch.h
>>> index e94870e77ee9..a967dbca3574 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
>>> * start of the packet or at the start of the l3 header depending on the
>>> value
>>> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>> * argument.
>>> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>> *
>>> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
>>> Not all
>>> * fields within a header are modifiable, e.g. the IPv4 protocol and
>>> fragment
>>> @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>>> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>>
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>>* from userspace. */
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index cab1e02b63e0..4ad9a45dc042 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -32,6 +32,7 @@
>>> #include "vport.h"
>>> #include "flow_netlink.h"
>>> #include "openvswitch_trace.h"
>>> +#include "drop.h"
>>>
>>> struct deferred_action {
>>> struct sk_buff *skb;
>>> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
>>> struct sk_buff *skb,
>>> return dec_ttl_exception_handler(dp, skb,
>>> key, a);
>>> break;
>>> +
>>> + case OVS_ACTION_ATTR_DROP:
>>> + u32 reason = nla_get_u32(a);
>>> +
>>> + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
>>> + SKB_DROP_REASON_SUBSYS_SHIFT;
>>> +
>>> + if (reason == OVS_XLATE_OK)
>>> + break;
>>> +
>>> + kfree_skb_reason(skb, reason);
>>> + return 0;
>>> }
>>>
>>> if (unlikely(err)) {
>>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>> index 41116361433d..23d39eae9a0d 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>> @@ -39,6 +39,7 @@
>>> #include
>>>
>>> #include "flow_netlink.h"
>>> +#include "drop.h"
>>>
>>> struct ovs_len_tbl {
>>> int len;
>>> @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr
>>> *actions)
>>> case OVS_ACTION_ATTR_RECIRC:
>>> case OVS_ACTION_ATTR_TRUNC:
>>> case OVS_ACTION_ATTR_USERSPACE:
>>> + case OVS_ACTION_ATTR_DROP:
>>> break;
>>>
>>> case OVS_ACTION_ATTR_CT:
>>> @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct
>>> nlattr *actions, int len)
>>> /* Whenever new actions are added, the need to update this
>>> * function should be considered.
>>> */
>>> - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
>>> + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
>>>
>>> if (!actions)
>>> return;
>>> @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net,
>>> const struct nlattr *attr,
>>> [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
>>> [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct
>>> ovs_action_add_mpls),
>>> [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
>>> + [OVS_ACTION_ATTR_DROP] = sizeof(u32),
>>> };
>>> const struct ovs_action_push_vlan
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Thu, Jul 06, 2023 at 08:54:16AM -0400, Aaron Conole wrote:
> Eric Garver writes:
>
> > This adds an explicit drop action. This is used by OVS to drop packets
> > for which it cannot determine what to do. An explicit action in the
> > kernel allows passing the reason _why_ the packet is being dropped. We
> > can then use perf tracing to match on the drop reason.
> >
> > e.g. trace all OVS dropped skbs
> >
> > # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> > [..]
> > 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> > location:0xc0d9b462, protocol: 2048, reason: 196610)
> >
> > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> >
> > Signed-off-by: Eric Garver
> > ---
> > include/uapi/linux/openvswitch.h| 2 ++
> > net/openvswitch/actions.c | 13 +
> > net/openvswitch/flow_netlink.c | 12 +++-
> > .../testing/selftests/net/openvswitch/ovs-dpctl.py | 3 +++
> > 4 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/openvswitch.h
> > b/include/uapi/linux/openvswitch.h
> > index e94870e77ee9..a967dbca3574 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
> > * start of the packet or at the start of the l3 header depending on the
> > value
> > * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> > * argument.
> > + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> > *
> > * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.
> > Not all
> > * fields within a header are modifiable, e.g. the IPv4 protocol and
> > fragment
> > @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> > + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> >
> > __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> >* from userspace. */
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index cab1e02b63e0..4ad9a45dc042 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -32,6 +32,7 @@
> > #include "vport.h"
> > #include "flow_netlink.h"
> > #include "openvswitch_trace.h"
> > +#include "drop.h"
> >
> > struct deferred_action {
> > struct sk_buff *skb;
> > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
> > struct sk_buff *skb,
> > return dec_ttl_exception_handler(dp, skb,
> > key, a);
> > break;
> > +
> > + case OVS_ACTION_ATTR_DROP:
> > + u32 reason = nla_get_u32(a);
> > +
> > + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > + SKB_DROP_REASON_SUBSYS_SHIFT;
> > +
> > + if (reason == OVS_XLATE_OK)
> > + break;
> > +
> > + kfree_skb_reason(skb, reason);
> > + return 0;
> > }
> >
> > if (unlikely(err)) {
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 41116361433d..23d39eae9a0d 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -39,6 +39,7 @@
> > #include
> >
> > #include "flow_netlink.h"
> > +#include "drop.h"
> >
> > struct ovs_len_tbl {
> > int len;
> > @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr
> > *actions)
> > case OVS_ACTION_ATTR_RECIRC:
> > case OVS_ACTION_ATTR_TRUNC:
> > case OVS_ACTION_ATTR_USERSPACE:
> > + case OVS_ACTION_ATTR_DROP:
> > break;
> >
> > case OVS_ACTION_ATTR_CT:
> > @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct
> > nlattr *actions, int len)
> > /* Whenever new actions are added, the need to update this
> > * function should be considered.
> > */
> > - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
> > + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
> >
> > if (!actions)
> > return;
> > @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net,
> > const struct nlattr *attr,
> > [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> > [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct
> > ovs_action_add_mpls),
> > [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> > + [OVS_ACTION_ATTR_DROP] = sizeof(u32),
> > };
> > const struct ovs_action_push_vlan *vlan;
> > int type =
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Eric Garver writes:
> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
>
> e.g. trace all OVS dropped skbs
>
> # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> [..]
> 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> location:0xc0d9b462, protocol: 2048, reason: 196610)
>
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>
> Signed-off-by: Eric Garver
> ---
> include/uapi/linux/openvswitch.h| 2 ++
> net/openvswitch/actions.c | 13 +
> net/openvswitch/flow_netlink.c | 12 +++-
> .../testing/selftests/net/openvswitch/ovs-dpctl.py | 3 +++
> 4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index e94870e77ee9..a967dbca3574 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -965,6 +965,7 @@ struct check_pkt_len_arg {
> * start of the packet or at the start of the l3 header depending on the
> value
> * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
> * argument.
> + * @OVS_ACTION_ATTR_DROP: Explicit drop action.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -1002,6 +1003,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> * from userspace. */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index cab1e02b63e0..4ad9a45dc042 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
> #include "vport.h"
> #include "flow_netlink.h"
> #include "openvswitch_trace.h"
> +#include "drop.h"
>
> struct deferred_action {
> struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> return dec_ttl_exception_handler(dp, skb,
>key, a);
> break;
> +
> + case OVS_ACTION_ATTR_DROP:
> + u32 reason = nla_get_u32(a);
> +
> + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> + SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> + if (reason == OVS_XLATE_OK)
> + break;
> +
> + kfree_skb_reason(skb, reason);
> + return 0;
> }
>
> if (unlikely(err)) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 41116361433d..23d39eae9a0d 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -39,6 +39,7 @@
> #include
>
> #include "flow_netlink.h"
> +#include "drop.h"
>
> struct ovs_len_tbl {
> int len;
> @@ -61,6 +62,7 @@ static bool actions_may_change_flow(const struct nlattr
> *actions)
> case OVS_ACTION_ATTR_RECIRC:
> case OVS_ACTION_ATTR_TRUNC:
> case OVS_ACTION_ATTR_USERSPACE:
> + case OVS_ACTION_ATTR_DROP:
> break;
>
> case OVS_ACTION_ATTR_CT:
> @@ -2394,7 +2396,7 @@ static void ovs_nla_free_nested_actions(const struct
> nlattr *actions, int len)
> /* Whenever new actions are added, the need to update this
>* function should be considered.
>*/
> - BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 23);
> + BUILD_BUG_ON(OVS_ACTION_ATTR_MAX != 24);
>
> if (!actions)
> return;
> @@ -3182,6 +3184,7 @@ static int __ovs_nla_copy_actions(struct net *net,
> const struct nlattr *attr,
> [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> [OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct
> ovs_action_add_mpls),
> [OVS_ACTION_ATTR_DEC_TTL] = (u32)-1,
> + [OVS_ACTION_ATTR_DROP] = sizeof(u32),
> };
> const struct ovs_action_push_vlan *vlan;
> int type = nla_type(a);
> @@ -3453,6 +3456,13 @@ static int __ovs_nla_copy_actions(struct net *net,
> const struct nlattr *attr,
> skip_copy = true;
> break;
>
> + case
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Fri, Jun 30, 2023 at 08:29:58AM -0400, Eric Garver wrote:
> On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> > On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > > This adds an explicit drop action. This is used by OVS to drop packets
> > > for which it cannot determine what to do. An explicit action in the
> > > kernel allows passing the reason _why_ the packet is being dropped. We
> > > can then use perf tracing to match on the drop reason.
> > >
> > > e.g. trace all OVS dropped skbs
> > >
> > > # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> > > [..]
> > > 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> > > location:0xc0d9b462, protocol: 2048, reason: 196610)
> > >
> > > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> > >
> > > Signed-off-by: Eric Garver
> >
> > ...
> >
> > > --- a/net/openvswitch/actions.c
> > > +++ b/net/openvswitch/actions.c
> > > @@ -32,6 +32,7 @@
> > > #include "vport.h"
> > > #include "flow_netlink.h"
> > > #include "openvswitch_trace.h"
> > > +#include "drop.h"
> > >
> > > struct deferred_action {
> > > struct sk_buff *skb;
> > > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
> > > struct sk_buff *skb,
> > > return dec_ttl_exception_handler(dp, skb,
> > >key, a);
> > > break;
> > > +
> > > + case OVS_ACTION_ATTR_DROP:
> > > + u32 reason = nla_get_u32(a);
> > > +
> > > + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > > + SKB_DROP_REASON_SUBSYS_SHIFT;
> > > +
> > > + if (reason == OVS_XLATE_OK)
> > > + break;
> > > +
> > > + kfree_skb_reason(skb, reason);
> > > + return 0;
> > > }
> >
> > Hi Eric,
> >
> > thanks for your patches. This is an interesting new feature.
> >
> > unfortunately clang-16 doesn't seem to like this very much.
> > I think that it is due to the declaration of reason not
> > being enclosed in a block - { }.
> >
> > net/openvswitch/actions.c:1483:4: error: expected expression
> > u32 reason = nla_get_u32(a);
> > ^
> > net/openvswitch/actions.c:1485:4: error: use of undeclared identifier
> > 'reason'
> > reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > ^
> > net/openvswitch/actions.c:1488:8: error: use of undeclared identifier
> > 'reason'
> > if (reason == OVS_XLATE_OK)
> > ^
> > net/openvswitch/actions.c:1491:26: error: use of undeclared identifier
> > 'reason'
> > kfree_skb_reason(skb, reason);
> > ^
> > 4 errors generated.
> >
> >
> > net-next is currently closed.
> > So please provide a v2 once it reposts, after 10th July.
>
> oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.
Thanks Eric,
much appreciated.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > This adds an explicit drop action. This is used by OVS to drop packets
> > for which it cannot determine what to do. An explicit action in the
> > kernel allows passing the reason _why_ the packet is being dropped. We
> > can then use perf tracing to match on the drop reason.
> >
> > e.g. trace all OVS dropped skbs
> >
> > # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> > [..]
> > 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> > location:0xc0d9b462, protocol: 2048, reason: 196610)
> >
> > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> >
> > Signed-off-by: Eric Garver
>
> ...
>
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -32,6 +32,7 @@
> > #include "vport.h"
> > #include "flow_netlink.h"
> > #include "openvswitch_trace.h"
> > +#include "drop.h"
> >
> > struct deferred_action {
> > struct sk_buff *skb;
> > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
> > struct sk_buff *skb,
> > return dec_ttl_exception_handler(dp, skb,
> > key, a);
> > break;
> > +
> > + case OVS_ACTION_ATTR_DROP:
> > + u32 reason = nla_get_u32(a);
> > +
> > + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > + SKB_DROP_REASON_SUBSYS_SHIFT;
> > +
> > + if (reason == OVS_XLATE_OK)
> > + break;
> > +
> > + kfree_skb_reason(skb, reason);
> > + return 0;
> > }
>
> Hi Eric,
>
> thanks for your patches. This is an interesting new feature.
>
> unfortunately clang-16 doesn't seem to like this very much.
> I think that it is due to the declaration of reason not
> being enclosed in a block - { }.
>
> net/openvswitch/actions.c:1483:4: error: expected expression
> u32 reason = nla_get_u32(a);
> ^
> net/openvswitch/actions.c:1485:4: error: use of undeclared identifier
> 'reason'
> reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> ^
> net/openvswitch/actions.c:1488:8: error: use of undeclared identifier
> 'reason'
> if (reason == OVS_XLATE_OK)
> ^
> net/openvswitch/actions.c:1491:26: error: use of undeclared identifier
> 'reason'
> kfree_skb_reason(skb, reason);
> ^
> 4 errors generated.
>
>
> net-next is currently closed.
> So please provide a v2 once it reposts, after 10th July.
oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.
Thanks.
Eric.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
>
> e.g. trace all OVS dropped skbs
>
> # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> [..]
> 106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> location:0xc0d9b462, protocol: 2048, reason: 196610)
>
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
>
> Signed-off-by: Eric Garver
...
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
> #include "vport.h"
> #include "flow_netlink.h"
> #include "openvswitch_trace.h"
> +#include "drop.h"
>
> struct deferred_action {
> struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> return dec_ttl_exception_handler(dp, skb,
>key, a);
> break;
> +
> + case OVS_ACTION_ATTR_DROP:
> + u32 reason = nla_get_u32(a);
> +
> + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> + SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> + if (reason == OVS_XLATE_OK)
> + break;
> +
> + kfree_skb_reason(skb, reason);
> + return 0;
> }
Hi Eric,
thanks for your patches. This is an interesting new feature.
unfortunately clang-16 doesn't seem to like this very much.
I think that it is due to the declaration of reason not
being enclosed in a block - { }.
net/openvswitch/actions.c:1483:4: error: expected expression
u32 reason = nla_get_u32(a);
^
net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
^
net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
if (reason == OVS_XLATE_OK)
^
net/openvswitch/actions.c:1491:26: error: use of undeclared identifier
'reason'
kfree_skb_reason(skb, reason);
^
4 errors generated.
net-next is currently closed.
So please provide a v2 once it reposts, after 10th July.
See:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
--
pw-bot: changes-requested
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Hi Eric, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Eric-Garver/net-openvswitch-add-drop-reasons/20230630-043307 base: net-next/main patch link: https://lore.kernel.org/r/20230629203005.2137107-3-eric%40garver.life patch subject: [PATCH net-next 2/2] net: openvswitch: add drop action config: hexagon-randconfig-r045-20230629 (https://download.01.org/0day-ci/archive/20230630/202306300609.tdrdzscy-...@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230630/202306300609.tdrdzscy-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306300609.tdrdzscy-...@intel.com/ All errors (new ones prefixed by >>): In file included from net/openvswitch/actions.c:8: In file included from include/linux/skbuff.h:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from net/openvswitch/actions.c:8: In file included from include/linux/skbuff.h:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from net/openvswitch/actions.c:8: In file included from include/linux/skbuff.h:17: In file included from include/linux/bvec.h:10: In file included from include/linux/highmem.h:12: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 584 | __raw_writeb(value, PCI_IOBASE + addr); | ~~ ^ include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~ ^ include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~ ^ >> net/openvswitch/actions.c:1483:4: error: expected expression 1483 |
Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action
Hi Eric,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url:
https://github.com/intel-lab-lkp/linux/commits/Eric-Garver/net-openvswitch-add-drop-reasons/20230630-043307
base: net-next/main
patch link:
https://lore.kernel.org/r/20230629203005.2137107-3-eric%40garver.life
patch subject: [PATCH net-next 2/2] net: openvswitch: add drop action
config: mips-randconfig-r036-20230629
(https://download.01.org/0day-ci/archive/20230630/202306300645.towwst4x-...@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git
f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce:
(https://download.01.org/0day-ci/archive/20230630/202306300645.towwst4x-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202306300645.towwst4x-...@intel.com/
All errors (new ones prefixed by >>):
>> net/openvswitch/actions.c:1483:4: error: expected expression
u32 reason = nla_get_u32(a);
^
>> net/openvswitch/actions.c:1485:4: error: use of undeclared identifier
>> 'reason'
reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
^
net/openvswitch/actions.c:1488:8: error: use of undeclared identifier
'reason'
if (reason == OVS_XLATE_OK)
^
net/openvswitch/actions.c:1491:26: error: use of undeclared identifier
'reason'
kfree_skb_reason(skb, reason);
^
4 errors generated.
vim +1483 net/openvswitch/actions.c
1285
1286 /* Execute a list of actions against 'skb'. */
1287 static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
1288struct sw_flow_key *key,
1289const struct nlattr *attr, int len)
1290 {
1291 const struct nlattr *a;
1292 int rem;
1293
1294 for (a = attr, rem = len; rem > 0;
1295 a = nla_next(a, )) {
1296 int err = 0;
1297
1298 if (trace_ovs_do_execute_action_enabled())
1299 trace_ovs_do_execute_action(dp, skb, key, a,
rem);
1300
1301 switch (nla_type(a)) {
1302 case OVS_ACTION_ATTR_OUTPUT: {
1303 int port = nla_get_u32(a);
1304 struct sk_buff *clone;
1305
1306 /* Every output action needs a separate clone
1307 * of 'skb', In case the output action is the
1308 * last action, cloning can be avoided.
1309 */
1310 if (nla_is_last(a, rem)) {
1311 do_output(dp, skb, port, key);
1312 /* 'skb' has been used for output.
1313 */
1314 return 0;
1315 }
1316
1317 clone = skb_clone(skb, GFP_ATOMIC);
1318 if (clone)
1319 do_output(dp, clone, port, key);
1320 OVS_CB(skb)->cutlen = 0;
1321 break;
1322 }
1323
1324 case OVS_ACTION_ATTR_TRUNC: {
1325 struct ovs_action_trunc *trunc = nla_data(a);
1326
1327 if (skb->len > trunc->max_len)
1328 OVS_CB(skb)->cutlen = skb->len -
trunc->max_len;
1329 break;
1330 }
1331
1332 case OVS_ACTION_ATTR_USERSPACE:
1333 output_userspace(dp, skb, key, a, attr,
1334 len,
OVS_CB(skb)->cutlen);
1335 OVS_CB(skb)->cutlen = 0;
1336 break;
1337
1338 case OVS_ACTION_ATTR_HASH:
1339 execute_hash(skb, key, a);
1340 break;
1341
1342 case OVS_ACTION_ATTR_PUSH_MPLS: {
1343 struct ovs_action_push_mpls *mpls = nla_data(a);
1344
1345 err = push_mpls(skb, key, mpls->mpls_lse,
1346 mpls->mpls_ethertype,
skb->mac_len);
1347 break;
1348 }
1349 case OVS_ACTION_ATTR_ADD_MPLS: {
1350 struct ovs_action_add_mpls *mpls = nla_data(a);
1351
