Re: [ovs-dev] [PATCH] ovs: Only clear tstamp when changing namespaces
On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki
wrote:
>
> As of "ovs: clear skb->tstamp in forwarding path", the
> tstamp is now being cleared unconditionally to fix fq qdisc
> operation with ovs vports.
>
> While this is mostly correct and fixes forwarding for that
> use case, a slight adjustment is necessary to ensure that
> the tstamp is cleared *only when the forwarding is across
> namespaces*.
Hmm? I am sure timestamp has already been cleared when
crossing netns:
void skb_scrub_packet(struct sk_buff *skb, bool xnet)
{
...
if (!xnet)
return;
ipvs_reset(skb);
skb->mark = 0;
skb->tstamp = 0;
}
So, what are you trying to fix?
>
> Signed-off-by: Tyler J. Stachecki
> ---
> net/openvswitch/vport.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index cf2ce5812489..c2d32a5c3697 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -507,7 +507,8 @@ void ovs_vport_send(struct vport *vport, struct sk_buff
> *skb, u8 mac_proto)
> }
>
> skb->dev = vport->dev;
> - skb->tstamp = 0;
> + if (dev_net(skb->dev))
Doesn't dev_net() always return a non-NULL pointer?
If you really want to check whether it is cross-netns, you should
use net_eq() to compare src netns with dst netns, something like:
if (!net_eq(dev_net(vport->dev), dev_net(skb->dev))).
Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs: Only clear tstamp when changing namespaces
On Sun, Sep 19, 2021 at 10:44 PM Tyler Stachecki wrote: > Sorry if this is a no-op -- I'm admittedly not familiar with this part > of the tree. I had added this check based on this discussion on the > OVS mailing list: > https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050966.html > > The motivation to add it was based on the fact that skb_scrub_packet > is doing it conditionally as well, but you seem to indicate that > skb_scrub_packet itself is already being done somewhere? I mean, skb->tstamp has been cleared when crossing netns, so: 1) you don't need to clear it again for this case; 2) clearly we fix other cases with commit 01634047bf0d, so your patch break it again. Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net v2 2/3] net/sched: flow_dissector: Fix matching on zone id for invalid conns
On Fri, Dec 10, 2021 at 8:52 PM Jakub Kicinski wrote:
>
> On Thu, 9 Dec 2021 09:57:33 +0200 Paul Blakey wrote:
> > @@ -238,10 +239,12 @@ void
> > skb_flow_dissect_ct(const struct sk_buff *skb,
> > struct flow_dissector *flow_dissector,
> > void *target_container, u16 *ctinfo_map,
> > - size_t mapsize, bool post_ct)
> > + size_t mapsize)
> > {
> > #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> > + bool post_ct = tc_skb_cb(skb)->post_ct;
> > struct flow_dissector_key_ct *key;
> > + u16 zone = tc_skb_cb(skb)->zone;
> > enum ip_conntrack_info ctinfo;
> > struct nf_conn_labels *cl;
> > struct nf_conn *ct;
> > @@ -260,6 +263,7 @@ skb_flow_dissect_ct(const struct sk_buff *skb,
> > if (!ct) {
> > key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED |
> > TCA_FLOWER_KEY_CT_FLAGS_INVALID;
> > + key->ct_zone = zone;
> > return;
> > }
> >
>
> Why is flow dissector expecting skb cb to be TC now?
> Please keep the appropriate abstractions intact.
Probably because only fl_classify() calls it, but I agree with you
on this point, this function is supposed to be TC independent.
Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] net: openvswitch: introduce common code for flushing flows
On Mon, Aug 10, 2020 at 6:14 PM wrote:
>
> From: Tonghao Zhang
>
> To avoid some issues, for example RCU usage warning, we should
> flush the flows under ovs_lock. This patch refactors
> table_instance_destroy and introduces table_instance_flow_flush
> which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
>
> Signed-off-by: Tonghao Zhang
Please add a Fixes tag here, I think it is probably your memory leak fix
which introduced this issue. And a Reported-by, to give credits to bug
reporters.
Plus one minor issue below:
> -static void table_instance_destroy(struct flow_table *table,
> - struct table_instance *ti,
> - struct table_instance *ufid_ti,
> - bool deferred)
> +/* Must be called with OVS mutex held. */
> +void table_instance_flow_flush(struct flow_table *table,
> + struct table_instance *ti,
> + struct table_instance *ufid_ti)
> {
> int i;
>
> - if (!ti)
> - return;
> -
> - BUG_ON(!ufid_ti);
> if (ti->keep_flows)
> - goto skip_flows;
> + return;
>
> for (i = 0; i < ti->n_buckets; i++) {
> - struct sw_flow *flow;
> struct hlist_head *head = &ti->buckets[i];
> struct hlist_node *n;
> + struct sw_flow *flow;
This is at most a coding style change, please do not mix
coding style changes in bug fixes. You can always push coding
style changes separately when net-next is open.
Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] net: openvswitch: introduce common code for flushing flows
On Wed, Aug 12, 2020 at 2:59 AM wrote:
>
> From: Tonghao Zhang
>
> To avoid some issues, for example RCU usage warning and double free,
> we should flush the flows under ovs_lock. This patch refactors
> table_instance_destroy and introduces table_instance_flow_flush
> which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
>
> Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy
> flow-table")
> Reported-by: Johan Knöös
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> Signed-off-by: Tonghao Zhang
Reviewed-by: Cong Wang
Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
