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;
From:   Song Liu <songliubraving@fb.com>
To:     Petr Mladek <pmladek@suse.com>
CC:     kernel test robot <lkp@intel.com>, Song Liu <song@kernel.org>,
        bpf <bpf@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        "live-patching@vger.kernel.org" <live-patching@vger.kernel.org>,
        "kbuild-all@lists.01.org" <kbuild-all@lists.01.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>
Subject: Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on
 the same function
Thread-Topic: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on
 the same function
Thread-Index: AQHYmmrwWVTrDYAzL0G5ckglE1ecDq2DvwKAgABeKwCAAD2SAA==
Date:   Mon, 18 Jul 2022 16:59:51 +0000
Message-ID: <9DAB0710-7D60-46AC-8A2F-ED4B8A1A4BC0@fb.com>
References: <20220718055449.3960512-3-song@kernel.org>
 <202207181552.VuKfz9zg-lkp@intel.com> <YtVd4FKOcEmGfubm@alley>
In-Reply-To: <YtVd4FKOcEmGfubm@alley>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?4HRIsPxRx4vEFbSwFkZBP1F8l7h4e46FMRzl5rENmK5A8IzJIhB29jQavV1u?=
 =?us-ascii?Q?nJpFO6drRBonP110CE8mDPcGZtDM9sQOHnVE5dDBH4styiMfbpWlaDSW9HP3?=
 =?us-ascii?Q?h4p/Tk6G7FKBBDJ6vQ5F+UknV645qGPYI5MpvqVwepjDFS7Bc7uLALPHYd+I?=
 =?us-ascii?Q?myO37jpCBlonJNSkzLFbSviN1BUfx0UPEr3NBsotlTPRzPNw8HoC73qsA3LC?=
 =?us-ascii?Q?mBnJL4KD6NRIT4eEV6oe+BwxZjGPZ5A8+6CEJRCfb5xlTqYQz1vcvRpPl+E9?=
 =?us-ascii?Q?3tlSzHD7jbRuk0OdSodCn9s0ZqZWEX9V3ljX9K6zRyrVVOZKdeW8I1HQmOH4?=
 =?us-ascii?Q?vo93zvAWkzzIGwBrDvgwHKTwnx7kkUmsAlGTjV24I9MSKbwJDugHIPjcDZrY?=
 =?us-ascii?Q?PhGNoU/H99SLmLDVYvywHgCAWvmvY0M09EKc8zztG7HwnzMiJHJGeL0mkoLq?=
 =?us-ascii?Q?GdtaGgsBsA9NVsjWvg2NKCiNQe/afOrNbP0P54U3axlZ7Y1eotBk82trX11+?=
 =?us-ascii?Q?iz+ICVkwWSULWui/zRSOlfUa9xFITRx20GIvD/mwvnqELmYoBIyt/ka9Duol?=
 =?us-ascii?Q?jdwNNGvbtgY8EdJPPY9rXlTWD+rNz998ROAV1w6oHHDj2d8TiBt1eG8Toe1P?=
 =?us-ascii?Q?y9zSGIVbVpPENi6+9juimtuiE9XHnvoIPM6jq7i9SKpwvG96W8HIykycC/Kv?=
 =?us-ascii?Q?je5ZfWQddNnoFtkiHuX1ep0VRq3lYU6fRkm0BSWXP78bUlHv/+00V6y7gkYV?=
 =?us-ascii?Q?dlI+/+IYrIuVE9by2nw1nPUwlR5grtTJnit0CPOO+X014EJx9aH4vqja0MGX?=
 =?us-ascii?Q?puG3CPi6OF/BZ5Dne9lUUWq+Kgkx0KAaRuORXyP96V/dqKbwcP/wFSdxc7+3?=
 =?us-ascii?Q?HzrCDnpO1cwXnqRVmxHTAP9Ok5t4LnMiHco3GthyOA8qQa90gwmAQ0KHzg5W?=
 =?us-ascii?Q?KmWcxiI0Pz2EVpW3S+CRsQMi7V9AI9p4o72pS9ZFy3XThUGmR9a/4hPUrGER?=
 =?us-ascii?Q?01TklhZK9sUtPVI3K6nMRRxtxEgFkTpbk9FvEJmGytxJmfbP/c+s+Gm1BUM5?=
 =?us-ascii?Q?NKCncnoNMA5cJRxOXaJVSsnqyHj55heZXjIsFMTFf418ocqciBLSOJQrwQJR?=
 =?us-ascii?Q?awWJbMATsnKHwjfuO1HwUMzwdDlwyNbg60JpulgRf8m84gFs82hten0L1Rxs?=
 =?us-ascii?Q?GWtpdL2a1A9bUvnz+37lCikRG0ypT+E9/Veb77orTjTLY+xxTY/kN0TMFeH9?=
 =?us-ascii?Q?hV7eJr6sNYJe+EXf7oQ1vl6QHeK3JfUvMCFjUMx5ItuPi/+36jVaqV0yruVc?=
 =?us-ascii?Q?6MGxQ6ZhMpboPGyIWQh/hjuEoDawe7Fp2xl15jW618kpFMNEMPGYV2nQByoJ?=
 =?us-ascii?Q?E6QmaakZ516rYEqN1pyhc1kxHH81w1rjcnEOa4uKSmjKvJnK0OFiHQyeciC4?=
 =?us-ascii?Q?8B/VJJp2Et+CoBv3BhuHaMZmiMFn5R3qROGWR4k20wG6+EzJ8JcljBrAw/P4?=
 =?us-ascii?Q?mWtAEuQS4HEamTBRMU0r2XvfUJWO1Mfh0kW8iSzYluhcuGkw5PwdB6lNllNg?=
 =?us-ascii?Q?gKmLFd4zUFhDMmbHn781Exv2tXc00Epsnjp1x73xKbuh96Q+KOBzi63C9VAc?=
 =?us-ascii?Q?xrOHdHwWrYAC/TsE1pv+Xpw=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5432BEE94F42FB4DB6B00AACF6B5C339@namprd15.prod.outlook.com>
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 78bc628d-23a7-4ae9-30fb-08da68deee0c
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jul 2022 16:59:51.1838
 (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: lYKkbhgnQ8YCik8+gQSp/wP2VETB07bY1uB3Rf8Y4gHaTLA0FswQq4XrWXEG+IiiddZ/hnB0Bw7snk4XYIReaw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3480
X-Proofpoint-ORIG-GUID: B7XsIwzn7mnmSqeepSTUN2orJ2LuWfOS
X-Proofpoint-GUID: B7XsIwzn7mnmSqeepSTUN2orJ2LuWfOS
X-Proofpoint-UnRewURL: 1 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-18_16,2022-07-18_01,2022-06-22_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        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 Jul 18, 2022, at 6:19 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Mon 2022-07-18 15:42:25, kernel test robot wrote:
>> Hi Song,
>> 
>> I love your patch! Perhaps something to improve:
>> 
>> [auto build test WARNING on bpf-next/master]
>> 
>> url:    https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
>> config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/202207181552.VuKfz9zg-lkp@intel.com/config )
>> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
>> reproduce (this is a W=1 build):
>>        # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
>>        git remote add linux-review https://github.com/intel-lab-lkp/linux
>>        git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
>>        git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
>>        # save the config file
>>        mkdir build_dir && cp config build_dir/.config
>>        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
>> 
>> 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 >>):
>> 
>>   kernel/trace/ftrace.c: In function 'register_ftrace_function':
>>>> kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
>>    8197 |         bool direct_mutex_locked = false;
>>         |              ^~~~~~~~~~~~~~~~~~~
>> 
>> 
>> vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
>> 
>>  8182	
>>  8183	/**
>>  8184	 * register_ftrace_function - register a function for profiling
>>  8185	 * @ops:	ops structure that holds the function for profiling.
>>  8186	 *
>>  8187	 * Register a function to be called by all functions in the
>>  8188	 * kernel.
>>  8189	 *
>>  8190	 * Note: @ops->func and all the functions it calls must be labeled
>>  8191	 *       with "notrace", otherwise it will go into a
>>  8192	 *       recursive loop.
>>  8193	 */
>>  8194	int register_ftrace_function(struct ftrace_ops *ops)
>>  8195		__releases(&direct_mutex)
>>  8196	{
>>> 8197		bool direct_mutex_locked = false;
>>  8198		int ret;
>>  8199	
>>  8200		ftrace_ops_init(ops);
>>  8201	
>>  8202		ret = prepare_direct_functions_for_ipmodify(ops);
>>  8203		if (ret < 0)
>>  8204			return ret;
>>  8205		else if (ret == 1)
>>  8206			direct_mutex_locked = true;
> 
> Honestly, this is another horrible trick. Would it be possible to
> call prepare_direct_functions_for_ipmodify() with direct_mutex
> already taken?
> 
> I mean something like:
> 
> 	mutex_lock(&direct_mutex);
> 
> 	ret = prepare_direct_functions_for_ipmodify(ops);
> 	if (ret)
> 		goto out:
> 
> 	mutex_lock(&ftrace_lock);
> 	ret = ftrace_startup(ops, 0);
> 	mutex_unlock(&ftrace_lock);
> 
> out:
> 	mutex_unlock(&direct_mutex);
> 	return ret;

Yeah, we can actually do something like this. We can also move the
ops->flags & FTRACE_OPS_FL_IPMODIFY check to 
register_ftrace_function(), so we only lock direct_mutex when when
it is necessary. 

> 
> 
>>  8208		mutex_lock(&ftrace_lock);
>>  8209	
>>  8210		ret = ftrace_startup(ops, 0);
>>  8211	
>>  8212		mutex_unlock(&ftrace_lock);
>>  8213	
> 
> Would be possible to handle tr->mutex the same way to avoid
> the trylock? I mean to take it in advance before direct_mutex?

Unfortunately, we cannot do this. ftrace code cannot look up 
bpf trampolines without locking direct_mutex. 

Thanks,
Song