diff mbox series

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

Message ID 20220812025015.316609-1-imagedong@tencent.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2] net: skb: prevent the split of kfree_skb_reason() by gcc | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1811025 this patch: 1811025
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 4539 this patch: 4541
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1868956 this patch: 1868956
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Menglong Dong Aug. 12, 2022, 2:50 a.m. UTC 
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(-)

Comments

Miguel Ojeda Aug. 12, 2022, 8:50 a.m. UTC | #1 
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
kernel test robot Aug. 13, 2022, 2:09 a.m. UTC | #2 
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
Menglong Dong Aug. 15, 2022, 6:20 a.m. UTC | #3 
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
diff mbox series

Patch

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;