netdev - Re: [PATCH net-next] openvswitch: Print error when ovs_execute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEYOeXN0QCahV_kAMZ8T2zwJiP_1gSnATBnCG7hA7XrGUT8cMA@mail.gmail.com>
Date:   Sun, 4 Aug 2019 19:48:16 -0700
From:   Yifeng Sun <pkusunyifeng@...il.com>
To:     Pravin Shelar <pshelar@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] openvswitch: Print error when
 ovs_execute_actions() fails

Yes, this fix is mainly for debugging purposes. If packets are
blackholed because
of errors from ovs_execute_actions(), we can got more helpful
information. Thanks
Pravin for the review, I will come up with a new version.
Yifeng

On Sat, Aug 3, 2019 at 4:00 PM Pravin Shelar <pshelar@....org> wrote:
>
> On Thu, Aug 1, 2019 at 2:14 PM Yifeng Sun <pkusunyifeng@...il.com> wrote:
> >
> > Currently in function ovs_dp_process_packet(), return values of
> > ovs_execute_actions() are silently discarded. This patch prints out
> > an error message when error happens so as to provide helpful hints
> > for debugging.
> >
> > Signed-off-by: Yifeng Sun <pkusunyifeng@...il.com>
> > ---
> >  net/openvswitch/datapath.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> > index 892287d..603c533 100644
> > --- a/net/openvswitch/datapath.c
> > +++ b/net/openvswitch/datapath.c
> > @@ -222,6 +222,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
> >         struct dp_stats_percpu *stats;
> >         u64 *stats_counter;
> >         u32 n_mask_hit;
> > +       int error;
> >
> >         stats = this_cpu_ptr(dp->stats_percpu);
> >
> > @@ -229,7 +230,6 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
> >         flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit);
> >         if (unlikely(!flow)) {
> >                 struct dp_upcall_info upcall;
> > -               int error;
> >
> >                 memset(&upcall, 0, sizeof(upcall));
> >                 upcall.cmd = OVS_PACKET_CMD_MISS;
> > @@ -246,7 +246,10 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
> >
> >         ovs_flow_stats_update(flow, key->tp.flags, skb);
> >         sf_acts = rcu_dereference(flow->sf_acts);
> > -       ovs_execute_actions(dp, skb, sf_acts, key);
> > +       error = ovs_execute_actions(dp, skb, sf_acts, key);
> > +       if (unlikely(error))
> > +               net_err_ratelimited("ovs: action execution error on datapath %s: %d\n",
> > +                                                       ovs_dp_name(dp), error);
> >
>
> I would rather add error counter for better visibility.
> If you want to use current approach, can you use net_dbg_ratelimited()
> since you want to use this for debugging purpose?
>
> Thanks.