netdev - Re: [PATCH v2 2/2] netcons: Add udp send fail statistics to netconsole

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO6EAnUe5-Yr=TE4Edi5oHenUR+mHYCh7ob7xu55V_dUn7d28w@mail.gmail.com>
Date: Fri, 30 Aug 2024 08:58:12 -0400
From: Maksym Kutsevol <max@...sevol.com>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] netcons: Add udp send fail statistics to netconsole

Hello Breno,


On Fri, Aug 30, 2024 at 4:45 AM Breno Leitao <leitao@...ian.org> wrote:
>
> Hello Maksym,
>
> On Wed, Aug 28, 2024 at 02:33:49PM -0700, Maksym Kutsevol wrote:
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 9c09293b5258..e14b13a8e0d2 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/inet.h>
> >  #include <linux/configfs.h>
> >  #include <linux/etherdevice.h>
> > +#include <linux/u64_stats_sync.h>
> >  #include <linux/utsname.h>
> >
> >  MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@...enic.com>");
>
> I am afraid that you are not build the patch against net-next, since
> this line was changed a while ago.
Yes, that's true. Jacub sent me the link to the net-tree specific
contribution doc, I also found
that. Will fix it in the next set.

> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=10a6545f0bdc
>
> Please develop on top of net-next, otherwise the patch might not apply
> on top of net-next.
>
> > +/**
> > + * netpoll_send_udp_count_errs - Wrapper for netpoll_send_udp that counts errors
> > + * @nt: target to send message to
> > + * @msg: message to send
> > + * @len: length of message
> > + *
> > + * Calls netpoll_send_udp and classifies the return value. If an error
> > + * occurred it increments statistics in nt->stats accordingly.
> > + * Noop if CONFIG_NETCONSOLE_DYNAMIC is disabled.
> > + */
> > +// static void netpoll_send_udp_count_errs(struct netpoll *np, const char *msg, int len)
>
> Have you forgot to remove the line above?
Yes, thank you.

> > +static void netpoll_send_udp_count_errs(struct netconsole_target *nt, const char *msg, int len)
> > +{
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +     int result = netpoll_send_udp(&nt->np, msg, len);
> > +     result = NET_XMIT_DROP;
>
> Could you please clarify why do you want to overwrite `result` here with
> NET_XMIT_DROP? It seems wrong.
Unfortunately I sent this patch with my debugging addons, this is plainly wrong.
Will remove.

> > +     if (result == NET_XMIT_DROP) {
> > +             u64_stats_update_begin(&nt->stats.syncp);
> > +             u64_stats_inc(&nt->stats.xmit_drop_count);
> > +             u64_stats_update_end(&nt->stats.syncp);
> > +     } else if (result == -ENOMEM) {
> > +             u64_stats_update_begin(&nt->stats.syncp);
> > +             u64_stats_inc(&nt->stats.enomem_count);
> > +             u64_stats_update_end(&nt->stats.syncp);
> > +     };
> > +#else
> > +     netpoll_send_udp(&nt->np, msg, len);
> > +#endif
>
> I am not sure this if/else/endif is the best way. I am wondering if
> something like this would make the code simpler (uncompiled/untested):
Two calls in two different places to netpoll_send_udp bothers you or
the way it has to distinct cases for enabled/disabled and you prefer to
have it as added steps for the case when it's enabled?


> static void netpoll_send_udp_count_errs(struct netconsole_target *nt, const char *msg, int len)
> {
>         int __maybe_unused result;
>
>         result = netpoll_send_udp(&nt->np, msg, len);
> #ifdef CONFIG_NETCONSOLE_DYNAMIC
>         switch (result) {
>         case NET_XMIT_DROP:
>                 u64_stats_update_begin(&nt->stats.syncp);
>                 u64_stats_inc(&nt->stats.xmit_drop_count);
>                 u64_stats_update_end(&nt->stats.syncp);
>                 breadk;
>         case ENOMEM:
>                 u64_stats_update_begin(&nt->stats.syncp);
>                 u64_stats_inc(&nt->stats.enomem_count);
>                 u64_stats_update_end(&nt->stats.syncp);
>                 break;
>         };
> #endif
>
> Thanks for working on it.
> --breno