[PATCH net-next v2] net: skb: prevent the split of kfree_skb_reason() by gcc

[PATCH net-next v2] net: skb: prevent the split of kfree_skb_reason() by gcc

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Sometimes, gcc will optimize the function by spliting it to two or
more functions. In this case, kfree_skb_reason() is splited to
kfree_skb_reason and kfree_skb_reason.part.0. However, the
function/tracepoint trace_kfree_skb() in it needs the return address
of kfree_skb_reason().

This split makes the call chains becomes:
  kfree_skb_reason() -> kfree_skb_reason.part.0 -> trace_kfree_skb()

which makes the return address that passed to trace_kfree_skb() be
kfree_skb().

Therefore, prevent this kind of optimization to kfree_skb_reason() by
making the optimize level to "O1". I think these should be better
method instead of this "O1", but I can't figure it out......

This optimization CAN happen, which depend on the behavior of gcc.
I'm not able to reproduce it in the latest kernel code, but it happens
in my kernel of version 5.4.119. Maybe the latest code already do someting
that prevent this happen?

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- replace 'optimize' with '__optimize__' in __nofnsplit, as Miguel Ojeda
  advised.
---
 include/linux/compiler_attributes.h | 2 ++
 net/core/skbuff.c                   | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 445e80517cab..b910b5775fc7 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -270,6 +270,8 @@
  */
 #define __noreturn                      __attribute__((__noreturn__))
 
+#define __nofnsplit                     __attribute__((__optimize__("O1")))
+
 /*
  * Optional: not supported by gcc.
  * Optional: not supported by icc.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 974bbbbe7138..ff9ccbc032b9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -777,7 +777,8 @@ EXPORT_SYMBOL(__kfree_skb);
  *	hit zero. Meanwhile, pass the drop reason to 'kfree_skb'
  *	tracepoint.
  */
-void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
+void __nofnsplit
+kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
 {
 	if (!skb_unref(skb))
 		return;
-- 
2.36.1

Re: [PATCH net-next v2] net: skb: prevent the split of kfree_skb_reason() by gcc

[Miguel Ojeda]

On Fri, Aug 12, 2022 at 4:50 AM <menglong8.dong@gmail.com> wrote:
>
>  #define __noreturn                      __attribute__((__noreturn__))
>
> +#define __nofnsplit                     __attribute__((__optimize__("O1")))

This is still in the wrong place...

Also, from what the bot says, Clang does not support it. I took a
look, and that seems to be the case. ICC doesn't, either. Thus you
would need to guard it and also add the docs as needed, like the other
attributes.

(Not saying that solving the issue with the attribute is a good idea,
but if you really wanted to add one, it should be done properly)

Cheers,
Miguel

Re: [PATCH net-next v2] net: skb: prevent the split of kfree_skb_reason() by gcc

[Menglong Dong]

Hello,

On Fri, Aug 12, 2022 at 4:50 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Aug 12, 2022 at 4:50 AM <menglong8.dong@gmail.com> wrote:
> >
> >  #define __noreturn                      __attribute__((__noreturn__))
> >
> > +#define __nofnsplit                     __attribute__((__optimize__("O1")))
>
> This is still in the wrong place...
>
> Also, from what the bot says, Clang does not support it. I took a
> look, and that seems to be the case. ICC doesn't, either. Thus you
> would need to guard it and also add the docs as needed, like the other
> attributes.
>
> (Not saying that solving the issue with the attribute is a good idea,
> but if you really wanted to add one, it should be done properly)
>

I have dug it deeper, and found that this function-split optimization
is only used by GCC. Therefore, I think I need only to consider it
for GCC.

I'll send a V3, thanks~

Menglong Dong

> Cheers,
> Miguel

Re: [PATCH net-next v2] net: skb: prevent the split of kfree_skb_reason() by gcc

[kernel test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/menglong8-dong-gmail-com/net-skb-prevent-the-split-of-kfree_skb_reason-by-gcc/20220812-105214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 7ebfc85e2cd7b08f518b526173e9a33b56b3913b
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220813/202208131003.M7FBGBna-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ec98116fd4b985103e65c71d47f9390fee025cb9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review menglong8-dong-gmail-com/net-skb-prevent-the-split-of-kfree_skb_reason-by-gcc/20220812-105214
        git checkout ec98116fd4b985103e65c71d47f9390fee025cb9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/core/skbuff.c:780:6: warning: unknown attribute '__optimize__' ignored [-Wunknown-attributes]
   void __nofnsplit
        ^~~~~~~~~~~
   include/linux/compiler_attributes.h:273:56: note: expanded from macro '__nofnsplit'
   #define __nofnsplit                     __attribute__((__optimize__("O1")))
                                                          ^~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +/__optimize__ +780 net/core/skbuff.c

   770	
   771	/**
   772	 *	kfree_skb_reason - free an sk_buff with special reason
   773	 *	@skb: buffer to free
   774	 *	@reason: reason why this skb is dropped
   775	 *
   776	 *	Drop a reference to the buffer and free it if the usage count has
   777	 *	hit zero. Meanwhile, pass the drop reason to 'kfree_skb'
   778	 *	tracepoint.
   779	 */
 > 780	void __nofnsplit
   781	kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
   782	{
   783		if (!skb_unref(skb))
   784			return;
   785	
   786		DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX);
   787	
   788		trace_kfree_skb(skb, __builtin_return_address(0), reason);
   789		__kfree_skb(skb);
   790	}
   791	EXPORT_SYMBOL(kfree_skb_reason);
   792	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp