Re: [PATCH net-next 1/7] net: ip: add drop reason to ip_route_input_noref()

[Menglong Dong]

On Sat, Oct 5, 2024 at 12:54 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> On Fri, Oct 4, 2024 at 6:36 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> >
> > no longer applies, please respin
> >
> > On Tue,  1 Oct 2024 13:59:59 +0800 Menglong Dong wrote:
> > > +     enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > >       const struct iphdr *iph = ip_hdr(skb);
> > > -     int err, drop_reason;
> > > +     int err;
> > >       struct rtable *rt;
> >
> > reverse xmas tree
> >
> > >
> > > -     drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > > -
> > >       if (ip_can_use_hint(skb, iph, hint)) {
> > >               err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos,
> > >                                       dev, hint);
> > > @@ -363,7 +362,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
> > >        */
> > >       if (!skb_valid_dst(skb)) {
> > >               err = ip_route_input_noref(skb, iph->daddr, iph->saddr,
> > > -                                        iph->tos, dev);
> > > +                                        iph->tos, dev, &drop_reason);
> >
> > I find the extra output argument quite ugly.
> > I can't apply this now to try to suggest something better, perhaps you
> > can come up with a better solution..
>
> Also, passing a local variable by address forces the compiler to emit
> more canary checks in more
> networking core functions.
>

Yeah, passing the address of the drop reasons to a function
looks not nice. The first glance for me is to make
ip_route_input_noref() return drop reasons, but I'm afraid that
the errno it returns is used by the caller.

Let me dig it deeper, and make the functions in this series
return drop reasons, instead of passing a local variable.

Thanks!
Menglong Dong


>
> See :
>
>
> config STACKPROTECTOR_STRONG
> bool "Strong Stack Protector"
> depends on STACKPROTECTOR
> depends on $(cc-option,-fstack-protector-strong)
> default y
> help
>   Functions will have the stack-protector canary logic added in any
>   of the following conditions:
>
>   - local variable's address used as part of the right hand side of an
>     assignment or function argument
>   - local variable is an array (or union containing an array),
>     regardless of array type or length
>   - uses register local variables
>
>   This feature requires gcc version 4.9 or above, or a distribution
>   gcc with the feature backported ("-fstack-protector-strong").
>
>   On an x86 "defconfig" build, this feature adds canary checks to
>   about 20% of all kernel functions, which increases the kernel code
>   size by about 2%.