1. Patchew
  2. linux
  3. View series

[PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging

Kriish Sharma posted 1 patch 2 months, 2 weeks ago
There is a newer version of this series
drivers/net/wan/hdlc_ppp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Kriish Sharma 2 months, 2 weeks ago 
Fixes warnings observed during compilation with -Wformat-overflow:

drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
  353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
  342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Update proto_name() to return "LCP" by default instead of NULL.
This change silences the compiler without changing existing behavior
and removes the need for the local 'pname' variable in ppp_cp_event.

Suggested-by: Krzysztof Hałasa <khalasa@piap.pl>
Fixes: 262858079afd ("Add linux-next specific files for 20250926")
Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
---
v2:
  - Target the net tree with proper subject prefix "[PATCH net]"
  - Update proto_name() to return "LCP" by default instead of NULL
  - Remove local 'pname' variable in ppp_cp_event
  - Add Suggested-by tag for Krzysztof Hałasa

v1: https://lore.kernel.org/all/20251002180541.1375151-1-kriish.sharma2006@gmail.com/

 drivers/net/wan/hdlc_ppp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 7496a2e9a282..281699e8d799 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -133,7 +133,7 @@ static inline const char *proto_name(u16 pid)
 	case PID_IPV6CP:
 		return "IPV6CP";
 	default:
-		return NULL;
+		return "LCP";
 	}
 }
 
-- 
2.34.1
Re: [PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Jakub Kicinski 2 months, 2 weeks ago 
On Fri,  3 Oct 2025 09:29:18 +0000 Kriish Sharma wrote:
> Fixes: 262858079afd ("Add linux-next specific files for 20250926")

This is not a fix, please post the next version in 2 weeks
(after the merge window has closed) and without the Fixes tag. 

The warning (not so?) obviously doesn't apply to the kernel. 
Kernel print methods will output "(null)" if you pass 0 as 
the value to %s.

I defer to Krzysztof on whether we should proceed with this
patch as a cleanup, but -Wformat-overflow= is disabled in 
the kernel builds for a good reason. This is not a fix.
-- 
pw-bot: cr
Re: [PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Krzysztof Hałasa 2 months, 2 weeks ago 
Hi Kriish,

Kriish Sharma <kriish.sharma2006@gmail.com> writes:

> --- a/drivers/net/wan/hdlc_ppp.c
> +++ b/drivers/net/wan/hdlc_ppp.c
> @@ -133,7 +133,7 @@ static inline const char *proto_name(u16 pid)
>         case PID_IPV6CP:
>                 return "IPV6CP";
>         default:
> -               return NULL;
> +               return "LCP";
>         }
>  }

I'd also remove the "PID_LCP" case as well (just those 2 lines
above IPCP case), the code will probably be simpler to understand
(the compiler won't care I guess).
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Re: [PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Kriish Sharma 2 months, 2 weeks ago 
Hi Chris,

Thanks for the suggestion. That makes sense I’ll drop the PID_LCP case
as well in v3 to simplify the code.
I’ll resend the patch targeting net-next once the merge window reopens.

Best regards,
Kriish

On Fri, Oct 3, 2025 at 6:03 PM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> Hi Kriish,
>
> Kriish Sharma <kriish.sharma2006@gmail.com> writes:
>
> > --- a/drivers/net/wan/hdlc_ppp.c
> > +++ b/drivers/net/wan/hdlc_ppp.c
> > @@ -133,7 +133,7 @@ static inline const char *proto_name(u16 pid)
> >         case PID_IPV6CP:
> >                 return "IPV6CP";
> >         default:
> > -               return NULL;
> > +               return "LCP";
> >         }
> >  }
>
> I'd also remove the "PID_LCP" case as well (just those 2 lines
> above IPCP case), the code will probably be simpler to understand
> (the compiler won't care I guess).
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
Re: [PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Simon Horman 2 months, 2 weeks ago 
On Fri, Oct 03, 2025 at 09:29:18AM +0000, Kriish Sharma wrote:
> Fixes warnings observed during compilation with -Wformat-overflow:
> 
> drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Update proto_name() to return "LCP" by default instead of NULL.
> This change silences the compiler without changing existing behavior
> and removes the need for the local 'pname' variable in ppp_cp_event.
> 
> Suggested-by: Krzysztof Hałasa <khalasa@piap.pl>
> Fixes: 262858079afd ("Add linux-next specific files for 20250926")

Perhaps this should be:

Fixes: e022c2f07ae5 ("WAN: new synchronous PPP implementation for generic HDLC.")
But more importantly, and sorry for not noticing this in my review of v1,
I'm not sure this is a bug fix. In his review of v1 Chris explains that
this case cannot be hit. And that the patch is about silencing the
compiler.

If so, I'd suggest this is a clean-up and thus you should consider:
1) Removing the fixes tag
2) Retargeting the patch at net-next

Please also note that net-next is currently closed for the merge window.
So any patches for it should be sent after it reopens, which will
be after v6.18-rc1 is released, most likely on or after 13th October.

See: https://docs.kernel.org/process/maintainer-netdev.html

The code change itself looks good to me.

> Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
> ---
> v2:
>   - Target the net tree with proper subject prefix "[PATCH net]"
>   - Update proto_name() to return "LCP" by default instead of NULL
>   - Remove local 'pname' variable in ppp_cp_event
>   - Add Suggested-by tag for Krzysztof Hałasa
> 
> v1: https://lore.kernel.org/all/20251002180541.1375151-1-kriish.sharma2006@gmail.com/
>
Re: [PATCH net v2] hdlc_ppp: fix potential null pointer in ppp_cp_event logging
Posted by Kriish Sharma 2 months, 2 weeks ago 
Hi Simon,

Thanks again for the clarification. I understand this is more of a
clean-up rather than a bug fix.
I’ll prepare a v3 targeting net-next once the merge window reopens,
removing the Fixes tag.

Best regards,
Kriish

On Fri, Oct 3, 2025 at 3:33 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Oct 03, 2025 at 09:29:18AM +0000, Kriish Sharma wrote:
> > Fixes warnings observed during compilation with -Wformat-overflow:
> >
> > drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> > drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Update proto_name() to return "LCP" by default instead of NULL.
> > This change silences the compiler without changing existing behavior
> > and removes the need for the local 'pname' variable in ppp_cp_event.
> >
> > Suggested-by: Krzysztof Hałasa <khalasa@piap.pl>
> > Fixes: 262858079afd ("Add linux-next specific files for 20250926")
>
> Perhaps this should be:
>
> Fixes: e022c2f07ae5 ("WAN: new synchronous PPP implementation for generic HDLC.")
> But more importantly, and sorry for not noticing this in my review of v1,
> I'm not sure this is a bug fix. In his review of v1 Chris explains that
> this case cannot be hit. And that the patch is about silencing the
> compiler.
>
> If so, I'd suggest this is a clean-up and thus you should consider:
> 1) Removing the fixes tag
> 2) Retargeting the patch at net-next
>
> Please also note that net-next is currently closed for the merge window.
> So any patches for it should be sent after it reopens, which will
> be after v6.18-rc1 is released, most likely on or after 13th October.
>
> See: https://docs.kernel.org/process/maintainer-netdev.html
>
> The code change itself looks good to me.
>
> > Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
> > ---
> > v2:
> >   - Target the net tree with proper subject prefix "[PATCH net]"
> >   - Update proto_name() to return "LCP" by default instead of NULL
> >   - Remove local 'pname' variable in ppp_cp_event
> >   - Add Suggested-by tag for Krzysztof Hałasa
> >
> > v1: https://lore.kernel.org/all/20251002180541.1375151-1-kriish.sharma2006@gmail.com/
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.