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: <d2af0d13-68cf-ad8c-5b16-af76201452c4@fb.com>
Date:   Thu, 10 Mar 2022 14:54:48 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.2
Subject: Re: [PATCH 1/2] bpf: Adjust BPF stack helper functions to accommodate
 skip > 0
Content-Language: en-US
To:     Namhyung Kim <namhyung@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>
Cc:     Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Eugene Loh <eugene.loh@oracle.com>, Hao Luo <haoluo@google.com>
References: <20220310082202.1229345-1-namhyung@kernel.org>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <20220310082202.1229345-1-namhyung@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cDkzdUdmNStTcm1QY1pYSjNGenZ1b09rVVEvcnh1eHdldWlQR0U3K1lwQ3BY?=
 =?utf-8?B?ZE1BaTdMOCtxem9sYk4zcWpaR3dEbW1NU2FlSFRmQWNIZFlFcWlBZkNnNmNt?=
 =?utf-8?B?ZXR4MXM1aHhLRTZzVFBOVERRV0dvMGwybjVsRitQY1RzNDNsZEdHdmM0bVlU?=
 =?utf-8?B?MTlpMG1GTlIrdGx5UC96Rlh0SFAzeVdWb3ZRZlFFOWNEaE1lR1F1ZWlvYVB0?=
 =?utf-8?B?RUtydFlJcTRlMGNnODRaMXhMVi83VGQ0Z2JYMGlLSWhjemFUMm5aWDRQbHdy?=
 =?utf-8?B?Y3kydWpvRSszZHJCS2JRL3dHaWF4d1pHY2I0MjdtY1JSZnd2SGlzWm1HcE9w?=
 =?utf-8?B?MDlYR25GbUhuOGE4U1lTRDZXSklIaVJ3YmQyZ0lwUTBDMUtDK2JHbDF4WHZZ?=
 =?utf-8?B?ZjVwZEw5VWE5WTJWeHBEcEZRTjNJamYvek1BZjRHTU1QWGM2RmhSTEhhRTFX?=
 =?utf-8?B?K3Qvc3U3MVRhOVBaVXErZDczSFpPUHZ3MjltVExUTWhMbHkrV0pKY0dPVkxL?=
 =?utf-8?B?NkhIMStxditJakdVTUVpaWVlc3I0N3JnNW5EbmtOalg0ZkZ5OVNZSWQ2alRo?=
 =?utf-8?B?TUhKbzVjcEFwMzZhOE1tNlhxTW9BeUI2SFZiR0xFUjk4VTdrcE1rQXV0d1ly?=
 =?utf-8?B?L1FkOWpBcVd3cDV2dy9YZVorWXNkUTNrYnFpUGp5UFdUUzhBOVNZTUVKSVk5?=
 =?utf-8?B?NkVUWDFUN2VvOHRkb1UxRmdaUEpmU1BhUlJ2bnY2ZjRrOGdDVFAzY096eTl5?=
 =?utf-8?B?dEFkSktIeno0VTV6OVJPeFlwS3QxVXh4WiszOXJDK2lIbitNUEJnVjlSQ2hS?=
 =?utf-8?B?Q1EwVkZsaDliSGJRRTFsYk05M1hWVmVEbDVaL0NoQTlmQU1IcHo4TTVsVXRL?=
 =?utf-8?B?eXlqZU1VTGVUWUtFSzVpRzdydGZjclpZdFdvbTFJaElqQ1BidEcrS2tyT3Jn?=
 =?utf-8?B?V3VDZ2wvSkdMbDYwQTNBTFZ0ajEzMnVMMnpkSkMxWE9EUzFZbWsxNWtWZmV5?=
 =?utf-8?B?R2dKU3E0QVZQZTBRY3NrMDlaYjNUMTQrQ2dUaUhKTlBBZU9hY2ROQi9naG4v?=
 =?utf-8?B?dDc0Z0xxbUZNdmw0amp2QzVOYWZRVjhnYmNqa2Nub0Uyd3E5bi9Xa0RTWHBm?=
 =?utf-8?B?dkVLSEtsOUF5aGg4VVpZM0gralRBZHRkaTBMWEFiblRQY01LMWExTWE2N0Zm?=
 =?utf-8?B?Vkw3c2xZZVE4YUlsM0pvcWlERS9iTUlJUmZscUtsejVkUmk0TW9KYkJzVGpN?=
 =?utf-8?B?NGR5UUlUUFZIaGVQSlRKanBkbFhxcVVVSzFGaSsySlR2eHVLMHcwZFBiUVU3?=
 =?utf-8?B?UThEZkRYbnNPdWExNzM4UmxzdjFZN09XN3BwRGVuYUQzenUvVjUxYTkxUmti?=
 =?utf-8?B?Q2dXMEVvd1dzZDNqQy9ELzBydEFpTE1Db2JncGxVdUVxLzQ0Qkw4SkdiTnNZ?=
 =?utf-8?B?R3R0ZVc0bVMyVnptTm9HemowTkROdjl0d1hVWDE3YWpnazlyS284U3VLVnI3?=
 =?utf-8?B?NFlDeC9vNnpKcTFrSEtjMlkydGRBWWp4L0k4NjRkWVBtWnk1aHZyZGVFRnFt?=
 =?utf-8?B?N2w4SzNyZmVGK3VVNzVBMlhRUXRuT25lTzJ1dUVQRGVLcGdQL2tBbkxreWlw?=
 =?utf-8?B?WEtKSzkxVUVPZ0EvQjRla0N1UWN5bGpBQjFZdmltRDhEb3ovZWd6bUgwWVpF?=
 =?utf-8?B?azgvc0xKUG40RUJFZzlkSHNvN0Z0VkFaZWxJS3U4d014akZYKzErWVlsVjJ3?=
 =?utf-8?B?QUxVNVM0VkNWb3FZU25HTWNzeGxBcWE5Rit6WE1WUVBhUC9td0J5b1V0dElN?=
 =?utf-8?B?UmtYVGd0QjV0SlpnL3NXenp4eWFqYmJlaEgyeHRQcE83V1Azdk1pa3RObGRx?=
 =?utf-8?B?aktNK2FjQ1c4aCtVWjROL0RjWHhDREI5WW9EK01TaU5vZ2x6Ly9ZSTVLb0tZ?=
 =?utf-8?B?WkpScXZpQ1FaUEdiZ0EyT0paYUVOblRKenZQVHFER3IwQmJ3MDlnWFY1SkNv?=
 =?utf-8?B?MFZqYmZOM1ZqelhadUE1d3ZPKy9lTzlvNmNmR0h3YVA3TUd4YVEwZ1lrSjJE?=
 =?utf-8?B?azNJT3gyWHJVMGIrVjVjQnk3Uy9QeTJHMzBOWG9Xd3FBLzhIQzJvbURlK3dE?=
 =?utf-8?B?Y216Q0hodXRYMloyWHFHa2tyaFVLTXdsczl6YjNmcnlNaHFjUFBsdjNiVGln?=
 =?utf-8?B?V3c9PQ==?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 830bfbe0-75fa-44fe-9dac-08da02e8fd05
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Mar 2022 22:54:52.8268
 (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: J3/N9AcKeTbd2nOhMa5rtqgA9E4xKE1lHXC+dDf5YHapFrxH7hQJgi8sILrrGSHg
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB4739
X-Proofpoint-GUID: CuJDOBX9s3O1AwA6VMgomitSzLBF6Gow
X-Proofpoint-ORIG-GUID: CuJDOBX9s3O1AwA6VMgomitSzLBF6Gow
Content-Transfer-Encoding: 7bit
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.64.514
 definitions=2022-03-10_09,2022-03-09_01,2022-02-23_01
X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        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_NONE,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 3/10/22 12:22 AM, Namhyung Kim wrote:
> Let's say that the caller has storage for num_elem stack frames.  Then,
> the BPF stack helper functions walk the stack for only num_elem frames.
> This means that if skip > 0, one keeps only 'num_elem - skip' frames.
> 
> This is because it sets init_nr in the perf_callchain_entry to the end
> of the buffer to save num_elem entries only.  I believe it was because
> the perf callchain code unwound the stack frames until it reached the
> global max size (sysctl_perf_event_max_stack).
> 
> However it now has perf_callchain_entry_ctx.max_stack to limit the
> iteration locally.  This simplifies the code to handle init_nr in the
> BPF callstack entries and removes the confusion with the perf_event's
> __PERF_SAMPLE_CALLCHAIN_EARLY which sets init_nr to 0.
> 
> Also change the comment on bpf_get_stack() in the header file to be
> more explicit what the return value means.
> 
> Based-on-patch-by: Eugene Loh <eugene.loh@oracle.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

The change looks good to me. This patch actually fixed a bug
discussed below:

 
https://lore.kernel.org/bpf/30a7b5d5-6726-1cc2-eaee-8da2828a9a9c@oracle.com/

A reference to the above link in the commit message
will be useful for people to understand better with an
example.

Also, the following fixes tag should be added:

Fixes: c195651e565a ("bpf: add bpf_get_stack helper")

Since the bug needs skip > 0 which is seldomly used,
and the current returned stack is still correct although
with less entries, I guess that is why less people
complains.

Anyway, ack the patch:
Acked-by: Yonghong Song <yhs@fb.com>


> ---
>   include/uapi/linux/bpf.h |  4 +--
>   kernel/bpf/stackmap.c    | 56 +++++++++++++++++-----------------------
>   2 files changed, 26 insertions(+), 34 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b0383d371b9a..77f4a022c60c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2975,8 +2975,8 @@ union bpf_attr {
>    *
>    * 			# sysctl kernel.perf_event_max_stack=<new value>
>    * 	Return
> - * 		A non-negative value equal to or less than *size* on success,
> - * 		or a negative error in case of failure.
> + * 		The non-negative copied *buf* length equal to or less than
> + * 		*size* on success, or a negative error in case of failure.
>    *
>    * long bpf_skb_load_bytes_relative(const void *skb, u32 offset, void *to, u32 len, u32 start_header)
[...]