linux-kernel - Re: [PATCH 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: <CAO6EAnU91bgDnWuih1BchpvcDhScDTviQKjf=sKamVwhvFmZcw@mail.gmail.com>
Date: Wed, 28 Aug 2024 11:04:00 -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 2/2] netcons: Add udp send fail statistics to netconsole

Hey Breno,
thanks for looking at it again :)


On Tue, Aug 27, 2024 at 8:18 AM Breno Leitao <leitao@...ian.org> wrote:
>
> On Sat, Aug 24, 2024 at 02:50:24PM -0700, Maksym Kutsevol wrote:
> > Enhance observability of netconsole. UDP sends can fail. Start tracking at
> > least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> > Stats are exposed via an additional attribute in CONFIGFS.
> >
> > The exposed statistics allows easier debugging of cases when netconsole
> > messages were not seen by receivers, eliminating the guesswork if the
> > sender thinks that messages in question were sent out.
> >
> > Stats are not reset on enable/disable/change remote ip/etc, they
> > belong to the netcons target itself.
> >
> > Signed-off-by: Maksym Kutsevol <max@...sevol.com>
>
> Would you mind adding a "Reported-by" me in this case?
>
> https://lore.kernel.org/all/ZsWoUzyK5du9Ffl+@gmail.com/

Absolutely!

> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 9c09293b5258..45c07ec7842d 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -82,6 +82,13 @@ static DEFINE_SPINLOCK(target_list_lock);
> >   */
> >  static struct console netconsole_ext;
> >
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +struct netconsole_target_stats  {
> > +     size_t xmit_drop_count;
> > +     size_t enomem_count;
>
> I am looking at other drivers, and they use a specific type for these
> counters, u64_stats_sync.
> if it is possible to use this format, then you can leverage the
> `__u64_stats_update` helpers, and not worry about locking/overflow!?
>
Do you think that these counters really need more than an int?
Switching them to unsigned int instead might be better?
I'd argue that at the point when an external system collection
interval is not short enough
to see the unsigned counter going to a lesser value (counters are
unsigned, they go to 0 at UINT_MAX+1).
I need advice/pointer on locking - I'm looking at it and it looks to
me as if there's no locking needed when
updating a member of nt there. Tell me if I'm wrong.

> > @@ -1015,6 +1035,25 @@ static struct notifier_block netconsole_netdev_notifier = {
> >       .notifier_call  = netconsole_netdev_event,
> >  };
> >
> > +/**
> > + * count_udp_send_stats - Classify netpoll_send_udp result and count errors.
> > + * @nt: target that was sent to
> > + * @result: result of netpoll_send_udp
> > + *
> > + * Takes the result of netpoll_send_udp and classifies the type of error that
> > + * occurred. Increments statistics in nt->stats accordingly.
> > + */
> > +static void count_udp_send_stats(struct netconsole_target *nt, int result)
> > +{
> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +     if (result == NET_XMIT_DROP) {
> > +             nt->stats.xmit_drop_count++;
>
> If you change the type, you can use the `u64_stats_inc` helper here.
ok

> > @@ -1126,7 +1167,11 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> >                       this_offset += this_chunk;
> >               }
> >
> > -             netpoll_send_udp(&nt->np, buf, this_header + this_offset);
> > +             count_udp_send_stats(nt,
> > +                                  netpoll_send_udp(&nt->np,
> > +                                                   buf,
> > +                                                   this_header + this_offset)
> > +             );
>
> as Jakub said, this is not a format that is common in the Linux kenrel.
ok