Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ca490974-0c5c-cfe9-0c6f-3ead163e7a7b@meta.com>
Date:   Sun, 18 Jun 2023 16:10:56 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: [PATCH bpf-next v5 2/3] bpf, x86: allow function arguments up to
 12 for TRACING
Content-Language: en-US
To:     menglong8.dong@gmail.com, alexei.starovoitov@gmail.com
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, benbjiang@tencent.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        Menglong Dong <imagedong@tencent.com>
References: <20230613025226.3167956-1-imagedong@tencent.com>
 <20230613025226.3167956-3-imagedong@tencent.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <20230613025226.3167956-3-imagedong@tencent.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Nk5neEFqeVBhcnFvdWp1RUhKZHViWkxMdXZ6dXlvRDZGNk14SDVNUy9vbUg4?=
 =?utf-8?B?TnB2QWxmY2QvZDVaNThZUjNxS3U0RlFDRW1YVkZIRU5EVVJDMmFPY2YzTzZ5?=
 =?utf-8?B?Y2F5YXFUczYveW9NbFhmZHB3R3VVYXA1WTRUdFN4YnhRbDk0d24rS0daRDh0?=
 =?utf-8?B?S0JoMXVDS2hnY2hra1dVR3dHY1p5VDBQSExIemtnTHVvcmFHM1hTOHhXQW9y?=
 =?utf-8?B?RlVvcm9NYnE4eElvSEI5bEpMeFZObWtXdmpLYnZlKzdPM0ZFNVlXWDVXUXFZ?=
 =?utf-8?B?MzRZbWMxbjIvLzZnRS9qOVNZcVErQ1NXOGFlMVNKY0Z3QUtxc2t1NmE1ZGx6?=
 =?utf-8?B?c2Jyakk0WHFVOUt0MzJkWDNDSkVsZ3hYd3pZWldHbWgzUi9HMjNISzkwZTg4?=
 =?utf-8?B?Wkg1bmZhSnE4aHZZWTJqa0FFSFRjODhnVGw1T21GOFhjTTJab0lXNHhrU1VI?=
 =?utf-8?B?Y0JWVkFqaSs1cmIrTzFpaDJqQnp4b2hyZjFRMWQwaEV3NC9hY2ppVzFCVkdM?=
 =?utf-8?B?TjNoNTlOdFhNY21qUmpUenhWRnBqREhKSDZXLytXRHhueHhsUzlJRmR5VW1v?=
 =?utf-8?B?TDBqNW9IQWttazNqaVM0WCtqWkh0dFRUR3ErRmZUNUVWWXpvZVBSVHJKUkdv?=
 =?utf-8?B?ZW9ESFJ5UkRRYk9MVUtoN3plZlc3Ly9jaFdEMnpVMDFXdWkweW9wSHlrWWhZ?=
 =?utf-8?B?d3czQzllY0pyVEhoamkwaVF5UGVvVmRLczdUVkpsQk5TR2hCRG5tNlBTNEd4?=
 =?utf-8?B?eWVWNUlEL3Z2aTZoMnVYcEZrRjl1QUx2Q2RpNVV6dElibC9wckkzSkx6YUFv?=
 =?utf-8?B?M3RQWVA0REMxa3hkeEZTVG53QmxiWlBmQkRCUTdoaTZZajlFaVJ6aGUySmVr?=
 =?utf-8?B?c2s0VWgxd0VwTnNWZzJMWG1vOU9lU0FSY3U3bzMwT29SenZTZVNpMWhjQXZW?=
 =?utf-8?B?L0I1LzhNaXhNZFBLdHZTTkdKU0hWTU84Tkg5aEN0YmtaZXh0UzQweEoyR0Nj?=
 =?utf-8?B?N09Db0tnOWcwYWlSL1ZqcXNjRGlRKzVyWVdVcnZ1dlN2c3hKMGNRTGhxNk8r?=
 =?utf-8?B?WHlFWFhDN2IxZkdUb0NTRFA4ZXRtTCszTnc3UHhhMHBQT1lmWFBBa3cwRml3?=
 =?utf-8?B?REUrUENYeDFTT0ZLc2NDSE5Dd0gzMDhKdDJmQ2piSEpTeUhDMzBUZFZ6QWlr?=
 =?utf-8?B?d0FjUUN6ZjZlT0JmVDA5cmpyYVBET1NGUU9YNitZZXF0N043Z2oyVzRob2Iv?=
 =?utf-8?B?T2JkR1FyUFdjRmpDS3M5Vi9tYVhwZTYvUElSN3Z1ZW5NOWk1V2NKYUl0cmFF?=
 =?utf-8?B?a0pHZ2k3OW9RRG5oayt1T282bDF1bmQvM09oYkVwOGIxejlLT3BWWnFZMlRH?=
 =?utf-8?B?VERGN3AveUNOTmdhVlVoa3NqQXhPejVFdUpLUzlzTi9rUk1Jbm55K1ZxeE5X?=
 =?utf-8?B?YjFHaWViYXdreVpYMDBwc3VnTGFvRjd2S3l2cGZ4eDNYMVRMa2dFN0FIb1dC?=
 =?utf-8?B?ZHZGY2dPSjFzNXhYckxQYlhaakxjN0RVWjNoSG5OdGI5L0YxSk1XVHppcHRB?=
 =?utf-8?B?UVoyRUk1NFJ6aWg4ditsT0xBS3RCeGNNbEk0M08rZ0cyekxzd2RiM2I3L3RG?=
 =?utf-8?B?b3FpeHdYVDN0d2Y3L08wdWtUK1RWMjJJQVV1TVlKSm1say9scldQY2QrbDZu?=
 =?utf-8?B?SW5QVHZJZytLY1NZTjZuOENjT0xjRThaRUdhc2xxNkxHMWY5Sm9mOWl4Ykhr?=
 =?utf-8?B?Z3FVWXNwMW1uQyt3T0pqOXIxcS94Tko3N2NjY2JGaVVDcURtYzU1d0ZqM2NQ?=
 =?utf-8?B?L2llSHNjNnQyZTkrK1k3RFlxdnVzSjZxdURzMjdPbWEyS1cwamFPalg3M3hO?=
 =?utf-8?B?VUtNSCtPTE9DQm5pdmFlVW1CSzlXU3hONjgrenpwQmNJb0lvV0NKMHl5UEpJ?=
 =?utf-8?B?ZWIzK2p2YWdLYXJHVEgyQVZoc0ZHS2pRZEt6WndLdTlDT2pjNnlLVTZPVENP?=
 =?utf-8?B?dnQ1enNvTVBKVjU5cGRHaWN5VDUrN1ZNK3Q0eHVKeTI4YzM4Qm04UmQyQ1c0?=
 =?utf-8?B?TmJxWWRodmZQa2ZpL1VKK0F1ZDBOZy9oQTRvb1YrUCttMDd2K3JDbUxTR2dt?=
 =?utf-8?B?dFBpZEtKZWFvQzJ2RHZZanQrRnkwUnRKRVVjRkFMZFVxS2NQUFlUcVZ0YTFv?=
 =?utf-8?B?b0E9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a242321a-c4f3-4eb8-1881-08db70514842
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jun 2023 23:11:01.1371
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mDELczqGImgp/Sf6RVKXRDF9NVvrHu0uHhJH7k4T5b8qmLJfZiMdXPqLLXNso2tM
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR15MB5394
X-Proofpoint-ORIG-GUID: e-PoyuIKaT9IjMRiR0c7A1Mv3oJgZLRa
X-Proofpoint-GUID: e-PoyuIKaT9IjMRiR0c7A1Mv3oJgZLRa
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-06-18_16,2023-06-16_01,2023-05-22_02
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/12/23 7:52 PM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> For now, the BPF program of type BPF_PROG_TYPE_TRACING can only be used
> on the kernel functions whose arguments count less than 6. This is not
> friendly at all, as too many functions have arguments count more than 6.
> 
> According to the current kernel version, below is a statistics of the
> function arguments count:
> 
> argument count | function count
> 7              | 704
> 8              | 270
> 9              | 84
> 10             | 47
> 11             | 47
> 12             | 27
> 13             | 22
> 14             | 5
> 15             | 0
> 16             | 1
> 
> Therefore, let's enhance it by increasing the function arguments count
> allowed in arch_prepare_bpf_trampoline(), for now, only x86_64.
> 
> For the case that we don't need to call origin function, which means
> without BPF_TRAMP_F_CALL_ORIG, we need only copy the function arguments
> that stored in the frame of the caller to current frame. The arguments
> of arg6-argN are stored in "$rbp + 0x18", we need copy them to
> "$rbp - regs_off + (6 * 8)".
> 
> For the case with BPF_TRAMP_F_CALL_ORIG, we need prepare the arguments
> in stack before call origin function, which means we need alloc extra
> "8 * (arg_count - 6)" memory in the top of the stack. Note, there should
> not be any data be pushed to the stack before call the origin function.
> Then, we have to store rbx with 'mov' instead of 'push'.

Please also mention special case related to 16-byte struct argument
in the comments of save_args().

> 
> We use EMIT3_off32() or EMIT4() for "lea" and "sub". The range of the
> imm in "lea" and "sub" is [-128, 127] if EMIT4() is used. Therefore,
> we use EMIT3_off32() instead if the imm out of the range.
> 
> It works well for the FENTRY/FEXIT/MODIFY_RETURN.
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v5:
> - consider the case of the struct in arguments can't be hold by regs
> v4:
> - make the stack 16-byte aligned if passing args on-stack is needed
> - add the function arguments statistics to the commit log
> v3:
> - use EMIT3_off32() for "lea" and "sub" only on necessary
> - make 12 as the maximum arguments count
> v2:
> - instead EMIT4 with EMIT3_off32 for "lea" to prevent overflow
> - make MAX_BPF_FUNC_ARGS as the maximum argument count
> ---
>   arch/x86/net/bpf_jit_comp.c | 221 +++++++++++++++++++++++++++++++-----
>   1 file changed, 195 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a407fbbffecd..47c699594dd8 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1857,37 +1857,165 @@ st:			if (is_imm8(insn->off))
>   	return proglen;
>   }
>   
> -static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> -		      int stack_size)
> +static inline void clean_stack_garbage(const struct btf_func_model *m,
> +				       u8 **pprog, int nr_args_on_stack,

nr_args_on_stack is actually nr_stack_slots, right? Maybe rename to
nr_stack_slots?

> +				       int stack_size)
>   {
> -	int i;
> +	int arg_size, off;
> +	u8 *prog;
> +
> +	if (nr_args_on_stack != 1)
> +		return;
> +
> +	/* the size of the last argument */
> +	arg_size = m->arg_size[m->nr_args - 1];
> +
> +	/* Generally speaking, the compiler will pass the arguments
> +	 * on-stack with "push" instruction, which will take 8-byte
> +	 * on the stack. On this case, there won't be garbage values

On this case -> In this case. The same for below another case.

> +	 * while we copy the arguments from origin stack frame to current
> +	 * in BPF_DW.
> +	 *
> +	 * However, sometimes the compiler will only allocate 4-byte on
> +	 * the stack for the arguments. For now, this case will only
> +	 * happen if there is only one argument on-stack and its size
> +	 * not more than 4 byte. On this case, there will be garbage
> +	 * values on the upper 4-byte where we store the argument on
> +	 * current stack frame.
> +	 *
> +	 * arguments on origin stack:
> +	 *
> +	 * stack_arg_1(4-byte) xxx(4-byte)
> +	 *
> +	 * what we copy:
> +	 *
> +	 * stack_arg_1(8-byte): stack_arg_1(origin) xxx
> +	 *
> +	 * and the xxx is the garbage values which we should clean here.
> +	 */

let us put the above comments before
 > +	if (nr_args_on_stack != 1)
 > +		return;


> +	if (arg_size <= 4) {
> +		off = -(stack_size - 4);
> +		prog = *pprog;
> +		/* mov DWORD PTR [rbp + off], 0 */
> +		if (!is_imm8(off))
> +			EMIT2_off32(0xC7, 0x85, off);
> +		else
> +			EMIT3(0xC7, 0x45, off);
> +		EMIT(0, 4);
> +		*pprog = prog;
> +	}
> +}
> +
> +static void save_args(const struct btf_func_model *m, u8 **prog,
> +		      int stack_size, bool on_stack)

Rename 'on_stack' to 'for_call_origin'? This should be more
clear about the use case.

> +{
> +	int arg_regs, first_off, nr_regs = 0, nr_stack = 0;
nr_stack -> nr_stack_slots?
> +	int i, j;
>   
>   	/* Store function arguments to stack.
>   	 * For a function that accepts two pointers the sequence will be:
>   	 * mov QWORD PTR [rbp-0x10],rdi
>   	 * mov QWORD PTR [rbp-0x8],rsi
>   	 */
> -	for (i = 0; i < min(nr_regs, 6); i++)
> -		emit_stx(prog, BPF_DW, BPF_REG_FP,
> -			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> -			 -(stack_size - i * 8));
> +	for (i = 0; i < min_t(int, m->nr_args, MAX_BPF_FUNC_ARGS); i++) {
> +		arg_regs = (m->arg_size[i] + 7) / 8;
> +
> +		/* According to the research of Yonghong, struct members
> +		 * should be all in register or all on the stack.
> +		 * Meanwhile, the compiler will pass the argument on regs
> +		 * if the remained regs can hold the argument.
remained -> remaining
> +		 *
> +		 * Disorder of the args can happen. For example:
> +		 *
> +		 * struct foo_struct {
> +		 *     long a;
> +		 *     int b;
> +		 * };
> +		 * int foo(char, char, char, char, char, struct foo_struct,
> +		 *         char);
> +		 *
> +		 * the arg1-5,arg7 will be passed by regs, and arg6 will
> +		 * by stack.
> +		 *
> +		 * Therefore, we should keep the same logic as here when
> +		 * we restore the regs in restore_regs.
> +		 */
> +		if (nr_regs + arg_regs > 6) {
> +			/* copy function arguments from origin stack frame
> +			 * into current stack frame.
> +			 *
> +			 * The starting address of the arguments on-stack
> +			 * is:
> +			 *   rbp + 8(push rbp) +
> +			 *   8(return addr of origin call) +
> +			 *   8(return addr of the caller)
> +			 * which means: rbp + 24
> +			 */
> +			for (j = 0; j < arg_regs; j++) {
> +				emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
> +					 nr_stack * 8 + 0x18);
> +				emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
> +					 -stack_size);
> +
> +				if (!nr_stack)
> +					first_off = stack_size;
> +				stack_size -= 8;
> +				nr_stack++;
> +			}
> +		} else {
> +			/* Only copy the arguments on-stack to current
> +			 * 'stack_size' and ignore the regs, used to
> +			 * prepare the arguments on-stack for orign call.
> +			 */
> +			if (on_stack) {
> +				nr_regs += arg_regs;
> +				continue;
> +			}
> +
> +			/* copy the arguments from regs into stack */
> +			for (j = 0; j < arg_regs; j++) {
> +				emit_stx(prog, BPF_DW, BPF_REG_FP,
> +					 nr_regs == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> +					 -stack_size);
> +				stack_size -= 8;
> +				nr_regs++;
> +			}
> +		}
> +	}
> +
> +	clean_stack_garbage(m, prog, nr_stack, first_off);
>   }
>   
[...]
>   	/* Generated trampoline stack layout:
> @@ -2147,7 +2286,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   	 *
>   	 * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
>   	 *
> +	 * RBP - rbx_off   [ rbx value       ]  always
> +	 *
>   	 * RBP - run_ctx_off [ bpf_tramp_run_ctx ]
> +	 *
> +	 *                     [ stack_argN ]  BPF_TRAMP_F_CALL_ORIG
> +	 *                     [ ...        ]
> +	 *                     [ stack_arg2 ]
> +	 * RBP - arg_stack_off [ stack_arg1 ]
>   	 */
>   
>   	/* room for return value of orig_call or fentry prog */
> @@ -2167,9 +2313,25 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>   
>   	ip_off = stack_size;
>   
> +	stack_size += 8;
> +	rbx_off = stack_size;
> +
>   	stack_size += (sizeof(struct bpf_tramp_run_ctx) + 7) & ~0x7;
>   	run_ctx_off = stack_size;
>   
> +	if (nr_regs > 6 && (flags & BPF_TRAMP_F_CALL_ORIG)) {
> +		stack_size += (nr_regs - 6) * 8;

Please double check. Is this okay for the case below?
   foo(int, int, int, int, int, 16_byte_struct)
here, nr_regs is 7, yes, to-be-increased stack size should be 2.


> +		/* make sure the stack pointer is 16-byte aligned if we
> +		 * need pass arguments on stack, which means
> +		 *  [stack_size + 8(rbp) + 8(rip) + 8(origin rip)]
> +		 * should be 16-byte aligned. Following code depend on
> +		 * that stack_size is already 8-byte aligned.
> +		 */
> +		stack_size += (stack_size % 16) ? 0 : 8;
> +	}
> +
> +	arg_stack_off = stack_size;
> +
[...]