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:     Steven Rostedt <rostedt@goodmis.org>
CC:     Song Liu <song@kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "live-patching@vger.kernel.org" <live-patching@vger.kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.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: AQHYmmrwWVTrDYAzL0G5ckglE1ecDq2GBfkAgABLI4A=
Date:   Tue, 19 Jul 2022 22:57:52 +0000
Message-ID: <C6229252-B41F-43B9-BABC-538947466710@fb.com>
References: <20220718055449.3960512-1-song@kernel.org>
 <20220718055449.3960512-3-song@kernel.org>
 <20220719142856.7d87ea6d@gandalf.local.home>
In-Reply-To: <20220719142856.7d87ea6d@gandalf.local.home>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?l4i7trkRJy1VUTyo6yLltAsFezJ5/SLMoDe/NMkwNag70jwg5j+jcDASrq4v?=
 =?us-ascii?Q?g73GyuYPrg/8KzP4yRU4zsynd/I7WqBFfZE+vx3DrcgV1rTRimAjXPMg0w09?=
 =?us-ascii?Q?3/R/3kv7jYN37JCYTBbkj3ywCHL7y0sWuBWHakZ6DE8Sxk4OqPK4PFan0Tp5?=
 =?us-ascii?Q?dL5pc3GdXAHyaodCRs6X3WX/zs6wUoEDH6pR6dwtnv0eSBY5byGbzS64wCZt?=
 =?us-ascii?Q?G8SKbkTjabxRdkWZqiz3lmjkunhAi2ZxWOnCcQ/LDmwm8Q830kZ07GfCVOVb?=
 =?us-ascii?Q?kYYOru/7uBlz9Akh587R64pUy0PToz4NzpMjkUxGGLyg+6UvviGlTYvLhaJP?=
 =?us-ascii?Q?6uLpAU7OgrEiMLMhDeyiBsrDdCBa//NOhe5UdDipE6kDrvebUdWVmKQjZY1a?=
 =?us-ascii?Q?92AAZ9ebdI+GQBXgGcIVl+3JhOg9C49X8oQ+U7HrEJ80Ipxj87Rk0dAruQkr?=
 =?us-ascii?Q?tCsMT/EGXi3EesGrh3J4jkjd4PSupX3dB9s7O4oCSd3ox5m7xBMbVBHCxGUZ?=
 =?us-ascii?Q?vSS+GjBPUrDGxZ7YLxqak78YXNW2yBBo6cKlFj/qjGGMfbtEGrlBhKDUMMZh?=
 =?us-ascii?Q?Prk9fqw8ExkrB6FwrT2iiCIyWJh+z67LT3KlmVFXOOBiQ6TaO2m8rwg6CPL1?=
 =?us-ascii?Q?bxCaeSLCAovC9MGaSz1qMKSMHj4l6Nq+luoe8lNlbaq/MTdA6S/9zgQJAtJc?=
 =?us-ascii?Q?WwrUqLLqhWIPPeXjPoTUL2yCZt73cOCR2WqClOgW0hQ52Ioc1z5O3vlaybC6?=
 =?us-ascii?Q?oVBHZ3c973vMOr0HZlny1/1Wq1YmD/XwetkgNtyh5XuhFlb7Gat+6mLBFKVk?=
 =?us-ascii?Q?C5wVmKNpxETI2O9sSC8NTgwu19Mu7W5ksQSxCqp/9Nb2o1hGNUd8X4EPDuk9?=
 =?us-ascii?Q?ueJggb1Jb1NPQlnGDvyzQh6PWWKOgwrrdwpw1TpNdvqRqSz+C6JZ3ny/nGSM?=
 =?us-ascii?Q?0xTCuyWgYsXIJrPERy2a40f90H3rN9g8zgk80m/4Z6ZiOShXWpmy7005WX40?=
 =?us-ascii?Q?Rh+/bCCCQbOH+lXh4RLdfrEcqVXehU/92+zA6a4yitD5vZ/DekqzLS74Sqks?=
 =?us-ascii?Q?7ckIOyuI+v3xTDvQQ2zBR2bi1lWByjsBMZqE1rwbuejA1VCdBckwRQd4AyUk?=
 =?us-ascii?Q?VOPkQ+4nuawr7ISoZVZn7nFRoRty6ks0vapB96wuZJRV2T+ydZO4942WJ/Mc?=
 =?us-ascii?Q?3FOCzhGzl0PbHuuHQebbPXuTIxBdZkGRZd6Dm6UuBImMABYGp7xN9yYRYT3y?=
 =?us-ascii?Q?shmIJlDZ1t2d19VcyF6iz6VfL3XKWjIXs1wcRqoDEouA9oV+tLn8kPmZO4kW?=
 =?us-ascii?Q?xj7Wy9MJ3RB2tEiDCSv43dcLom38/n6Xee/2KSKgLYsDcmUinjdNvyik9E65?=
 =?us-ascii?Q?V8jR2TbIY5aYsVV8rZh2WRs6Sg45skWjux2hviPMN8CcCZbSErBBvTc48gQd?=
 =?us-ascii?Q?EUzKWXdJcN70iq5/NLJcihCk6pmYHz6mAYDU630nEUliw06HgIhc5jc/K8Ua?=
 =?us-ascii?Q?3QRdo/0WzS47FLu7tJehTYqLAb7gLyGUfpoi95lBUFPqTmqcFYGPKL/CfoZ8?=
 =?us-ascii?Q?r+hXhBxdOG8DB+byhlf3Ni1xvanAIiTwnZpUHX3QAd3fhD0j+4cUgze8j5Sg?=
 =?us-ascii?Q?mdO+qpOsw3Kqk/BrlTWBI9Q=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C0FB8228F3C90C468745A56B944765D9@namprd15.prod.outlook.com>
MIME-Version: 1.0
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: e54d0753-00bc-4b99-ad30-08da69da1c50
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2022 22:57:52.4869
 (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: rt+3zlCw8vCisOebjV1kDK9cx11kCNFUtHQkbCRYH5F9uAHsn1l+G+5lj1cq/z2bRWPgCOWY1d/nJKkujNLz2A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR15MB4632
X-Proofpoint-ORIG-GUID: mEbU5ll4ChLZRZvr-a23lNhm29q9qCTY
X-Proofpoint-GUID: mEbU5ll4ChLZRZvr-a23lNhm29q9qCTY
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-19_10,2022-07-19_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 19, 2022, at 11:28 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Sun, 17 Jul 2022 22:54:47 -0700
> Song Liu <song@kernel.org> wrote:
> 
> Again, make the subject:
> 
>  ftrace: Allow IPMODIFY and DIRECT ops on the same function
> 
Will fix. 

> [...]

>> +
>> +/*
>> + * For most ftrace_ops_cmd,
>> + * Returns:
>> + *        0 - Success.
>> + *        -EBUSY - The operation cannot process
>> + *        -EAGAIN - The operation cannot process tempoorarily.
> 
> Just state:
> 
> 	Returns:
> 		0 - Success
> 		Negative on failure. The return value is dependent
> 		on the callback.
> 
> Let's not bind policy of the callback with ftrace.

Will fix. 

> 
>> + */
>> +typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
>> +
>> #ifdef CONFIG_DYNAMIC_FTRACE
>> /* The hash used to know what functions callbacks trace */
>> 

[...]

>> 
>> -	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>> +	is_ipmodify = ops->flags & FTRACE_OPS_FL_IPMODIFY;
>> +	is_direct = ops->flags & FTRACE_OPS_FL_DIRECT;
>> +
>> +	/* either IPMODIFY nor DIRECT, skip */
>> +	if (!is_ipmodify && !is_direct)
>> 		return 0;
> 
> I wonder if we should also add:
> 
> 	if (WARN_ON_ONCE(is_ipmodify && is_direct))
> 		return 0;
> 
> As a direct should never have an ipmodify.

Right, I will also remove IPMODIFY from direct_ops:

@ -2487,8 +2490,7 @@ static void call_direct_funcs(unsigned long ip, unsigned long pip,

 struct ftrace_ops direct_ops = {
        .func           = call_direct_funcs,
-       .flags          = FTRACE_OPS_FL_IPMODIFY
-                         | FTRACE_OPS_FL_SAVE_REGS
+       .flags          = FTRACE_OPS_FL_SAVE_REGS
                          | FTRACE_OPS_FL_PERMANENT,
        /*
         * By declaring the main trampoline as this trampoline


> 
>> 
>> 	/*
>> -	 * Since the IPMODIFY is a very address sensitive action, we do not
>> -	 * allow ftrace_ops to set all functions to new hash.
>> +	 * Since the IPMODIFY and DIRECT are very address sensitive
>> +	 * actions, we do not allow ftrace_ops to set all functions to new
>> +	 * hash.

[...]

> 
> Again, these are ops_func() specific and has nothing to do with the logic
> in this file. Just state:
> 
> * Returns:
> *         0 - @ops does not have IPMODIFY or @ops itself is DIRECT, no
> *             change needed;
> *         1 - @ops has IPMODIFY, hold direct_mutex;
> *         Negative on error.
> 
> And if we move the logic that this does not keep hold of the direct_mutex,
> we could just let the callback return any non-zero on error.
> 
>> + */
>> +static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
>> +	__acquires(&direct_mutex)
>> +{
>> +	struct ftrace_func_entry *entry;
>> +	struct ftrace_hash *hash;
>> +	struct ftrace_ops *op;
>> +	int size, i, ret;
>> +
>> +	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>> +		return 0;
>> +
>> +	mutex_lock(&direct_mutex);
>> +
>> +	hash = ops->func_hash->filter_hash;
>> +	size = 1 << hash->size_bits;
>> +	for (i = 0; i < size; i++) {
>> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
>> +			unsigned long ip = entry->ip;
>> +			bool found_op = false;
>> +
>> +			mutex_lock(&ftrace_lock);
>> +			do_for_each_ftrace_op(op, ftrace_ops_list) {
>> +				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
>> +					continue;
>> +				if (ops_references_ip(op, ip)) {
>> +					found_op = true;
>> +					break;
> 
> I think you want a goto here. The macros "do_for_each_ftrace_op() { .. }
> while_for_each_ftrace_op()" is a double loop. The break just moves to the
> next set of pages and does not break out of the outer loop.

Hmmm... really? I didn't see it ...


#define do_for_each_ftrace_op(op, list)                 \
        op = rcu_dereference_raw_check(list);                   \
        do

#define while_for_each_ftrace_op(op)                            \
        while (likely(op = rcu_dereference_raw_check((op)->next)) &&    \
               unlikely((op) != &ftrace_list_end))

Did I miss something...?

> 
> 					goto out_loop;
> 
>> +				}
>> +			} while_for_each_ftrace_op(op);

[...]

> 
>> 	mutex_lock(&ftrace_lock);
>> 
>> 	ret = ftrace_startup(ops, 0);
>> 
>> 	mutex_unlock(&ftrace_lock);
>> 
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>> +	if (direct_mutex_locked)
>> +		mutex_unlock(&direct_mutex);
>> +#endif
> 
> Change this to:
> 
> out_unlock:
> 	mutex_unlock(&direct_mutex);
> 

We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as 
direct_mutex is not defined without that config. 

Thanks,
Song

[...]