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: <526b46a7-7daa-9050-1276-e19836816991@oracle.com>
Date:   Mon, 16 May 2022 02:00:50 +1000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.9.0
Subject: Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected.
Content-Language: en-US
To:     Tejun Heo <tj@kernel.org>
Cc:     viro@zeniv.linux.org.uk, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org
References: <20220428055431.3826852-1-imran.f.khan@oracle.com>
 <20220428055431.3826852-3-imran.f.khan@oracle.com>
 <YnQtL7+GYHwpo4n2@slm.duckdns.org>
 <f5ff2554-580c-c817-b77f-25e2ef46411f@oracle.com>
 <Yn0/RrgMRie2YPEb@slm.duckdns.org>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <Yn0/RrgMRie2YPEb@slm.duckdns.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V2tra1RXdXMwVFArdDhId1NhajErZCtjMDNRR00zTzhvK0ltY25ndzVFcVZH?=
 =?utf-8?B?TDRjZjh4RzJvZTJEeVRmR1B3YzdEc044bTNqS1M5Z04vN2NmZXRXUWhsVE15?=
 =?utf-8?B?QjhFOG1QaXpmUTY4bk5WcWZaMVo3UGRvSHNaaytZWSsrMXpvRjhVcm9COWg3?=
 =?utf-8?B?MUJuN1MwVytybjJkSUhlbzZyRUZkL1grcGFKUFdTT2hIdkpmUHpwd0c2alBF?=
 =?utf-8?B?Y0c4R2p3V0FmWk53RWtzQ0ROaXBZY3ZGeW0wV3ZLR2NuczRBaFVtUWNhZUxi?=
 =?utf-8?B?Z2xkSFVpRCswYVV5VEZiL3dXZXNNQ0JYU1Z5QU9veHZvZEUrcXZSOWtIVGty?=
 =?utf-8?B?RHZCZDgxejAvTURXSUJLQVlrd2huTWhaWGd1LzIvWEh6MjI3Y1p6TkNqZmVy?=
 =?utf-8?B?WnlvaDM1TVRmaWwrc3R1ZWpqVFRYN3dLYWRlWWxWWVZTYlRLSFFsQ2drdURq?=
 =?utf-8?B?M0RsNmFqU0R2MHNMV2RxbEl4Tit4WW1Za3haek9hLzF4UVFjc1lzQnZxaWp0?=
 =?utf-8?B?Ui9yaDJ0NzArTTk5SUswTUpBUVpyWkcvcUJZUVI5cDlJWDUvdCtUZ1BGbVZm?=
 =?utf-8?B?STJzUEk5OXBRWlRVdkRoYXlUMW9keUVubFBFT1dlQW1hL2dLRFMrMVF0ZFNx?=
 =?utf-8?B?S1BxT0lXdUp3bm9QQzNuSDdHTURpSmR6cDc0SjhTNHNGZUw4akFhTm9WNzky?=
 =?utf-8?B?bGZ4YWNwNUFJTmRUa3B1Q3F1WGU5bTdHdWZ4eTBXWGZmcDVPVERxMkE1Rm9z?=
 =?utf-8?B?UjJ4czJmeE9kNWZFK0J6M0hHVkh4QUNXOUVsbFhxYTN2ZTBkRnR2OGs3dG1I?=
 =?utf-8?B?STBZUmdKMUw4YndOOThDUWJvOEZvSFNOWFIrTjNOYlFhVUNKN3U4ZUx0REdP?=
 =?utf-8?B?WWIzd1ZyV0dJSjV3eStHTWlNL1R4NzRGa0dMbmE1endLNXgxSFU5b1hQS0pD?=
 =?utf-8?B?TWdmWndEMTEwSWducklGMWd5WlE0bnVQRFljMTlrcTRuYUROeU80MnBXd3o1?=
 =?utf-8?B?WTl1THhPVWNpWkVaOUplL0lPbDJybnJUMUFTTjRkQ2dhSFBWL21NRTFCdnZp?=
 =?utf-8?B?YXNSeGw0Y0pNMVI3TTQzYXc0MjZoQlcwQm1oZ0lDVkYvbUpFTkUvaVFyL2g0?=
 =?utf-8?B?d2o0bFBEMmc0U3lPckxJZUlzWjV5ZjBFUFAxZ1NIcHdZMW5ZYk15Y3MxVmc0?=
 =?utf-8?B?SGwvQlc0Q2hMK0xNQXNLNEtiZTE4OG9XL2llVDZaUGVYQkp3SjRPeFFwSjVp?=
 =?utf-8?B?UmZyN0JILzhRb1VwdXdjcXpYcUlTTmhtUEllcWJ4cFBwemYwanFmbllkWk53?=
 =?utf-8?B?M1BNS1dRTmlMZmpUNFdwT0gzeHVWb05namxyK21zTmQrbExaTlBNNTdsdWkw?=
 =?utf-8?B?NWZVYkVhNndXMTlsQXZzZmVpWTg1M0VQZFU3bnAwd3VSS3VpVDRvaG5tWldJ?=
 =?utf-8?B?bStNeEdoZHErTkdyNFQxa1FaNDZ5dlFkSlN2K29Md29NbCtNVWRKWkVKbkJW?=
 =?utf-8?B?MUxodzE4RG5zVVZWblBLR053K21WQ2RJblBVNVF4TXVRenVzUXEvVGhLNk1T?=
 =?utf-8?B?UDMvZ2p2UzBPeWRVZnNSS0lVSkgyTERYd2FtcDZzZW93MHI1eXc0cXNVMDRV?=
 =?utf-8?B?SFU4MVpZS0ZZRjhVMUFkbDFreDFEZmFCbzFnSHc3UWZuTkdsa1FxbmE2VjV2?=
 =?utf-8?B?Zmc0WURqWENCbFlGQVBDTEExZzg0dTFTWGxXZUlxSTBHMit0UkZacmx1c1F6?=
 =?utf-8?B?dmpaTllVMDl4RDR1L3FrMUltQWFyZlZBMFdlR0Z3Sk1PQk90MkZ4Rk1Bb2Ur?=
 =?utf-8?B?TUx4YnIreGU5TDNJYlR0TjZMKzV2L2tNQWN6M25lbjlybjlHWlRPMVN3M2Yx?=
 =?utf-8?B?U212Q01kdW1jejV3bDA4bzVjVkw1eFMwZU5CSU5oWEQ2OGJjNEpTN1FpcVZZ?=
 =?utf-8?B?NEZwazZGQVFpREVjRDI2TVpOSlFQK296dlJlR3Jla1MrRUFzMG5LdUxhd2VR?=
 =?utf-8?B?Y09MajBsV3hESCtRMVdaQlZuczhub01WSXo3ejJVK1Azczl1OWNrY0k1SGg2?=
 =?utf-8?B?TlNDblQvZDhvYURvMjdUS0dtbEtCelQ1VmsvRjgxZnFQdnJwMVp3d3JsbUM3?=
 =?utf-8?B?Vm84bGFQZ0xNam1XeUV3MThFdXJJc0tSeFppSVlGSkRQdThsQzZtaXQ4NkNr?=
 =?utf-8?B?VGREd0JMYXhvUWd3SUZ4bEViWmdEMEhlV0ZXT1g5M01WVjhrZGhTRE4yakl6?=
 =?utf-8?B?czBZdHpCZUM0VHM0S2xOa2wrd2E5VXZpaVJib051bmE0aU1jOG4zKzJoUTRq?=
 =?utf-8?B?U0VkYXpyeTdnWS9xV3hOR2hWU2YrOEJ5Q3dWM1Z6OXJXSTd5eE1EYi9tOHQ2?=
 =?utf-8?Q?I+oV0kswyDnGap0Q=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 687de138-f996-4baa-7b4c-08da368c197e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2022 16:00:58.8478
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WG+LdSDSG5JZlMBogUE4vm0TnLPoph00WfJezoek9PcnXAtrEkJlvCYd7rMQSCcOMylXc3A5wCYklUW6Us/wrg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR10MB6110
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-15_08:2022-05-13,2022-05-15 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 spamscore=0
 bulkscore=0 malwarescore=0 adultscore=0 suspectscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205150089
X-Proofpoint-GUID: wcW_8Lx6LBbTH1vQ545b24uzYDewUWsG
X-Proofpoint-ORIG-GUID: wcW_8Lx6LBbTH1vQ545b24uzYDewUWsG
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        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

Hello Tejun,

Thanks for your reply.

On 13/5/22 3:09 am, Tejun Heo wrote:
> Hello,
> 
> On Fri, May 06, 2022 at 09:12:23AM +1000, Imran Khan wrote:
>> On 6/5/22 6:01 am, Tejun Heo wrote:
>>> On Thu, Apr 28, 2022 at 03:54:28PM +1000, Imran Khan wrote:
>>>> +static struct kernfs_open_node *kernfs_deref_on_raw(struct kernfs_node *kn)
>>>> +{
>>>> +	return rcu_dereference_raw(kn->attr.open);
>>>> +}
>>>
>>> Wrapping the above probably isn't helping anything.
>>>
>>
>> This change is using raw deref at a few places, so I thought of putting
>> raw deref under a wrapper and explain in the wrapper function comment
>> under what conditions raw dereferencing was safe.
> 
> I don't think they need raw deref in the first place and if you *really*
> need raw deref, the wrapper explaining why they're needed and how they're
> safe is the whole point of it and I don't think the wrapper achieves that.
> 

Okay. I checked with rcu_access_pointer and CONFIG_PROVE_RCU enabled and did not
observe any warning(s) so rcu_access_pointer should have sufficed here. But I am
using rcu_dereference_check to accommodate checking of @of->list as well. The
checking of @of->list also covers one of your later suggestions. I have updated
the description of function as well. Could you please let me know if below looks
okay:

+/*
+ * Deref RCU protected kn->attr.open.
+ * If both @of->list and @kn->attr.open->files are non empty, we can safely
+ * assume that @of is on @kn->attr.open and hence @kn->attr.open will not
+ * vanish and derefeencing is safe here.
+ */
+static struct kernfs_open_node *
+kernfs_deref_on_check(struct kernfs_open_file *of, struct kernfs_node *kn)
+{
+       struct kernfs_open_node *on;
+
+       on = rcu_dereference_check(kn->attr.open, !list_empty(&of->list));
+
+       if (on && list_empty(&on->files))
+               return NULL;
+       else
+               return on;
+}
+

If this looks okay then I can replace usage of kernfs_deref_on_raw with
kernfs_deref_on_check.

>>>> +/*
>>>> + * Check ->attr.open corresponding to @kn while holding kernfs_open_file_mutex.
>>>> + * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely
>>>> + * accessed outside RCU read-side critical section, while holding the mutex.
>>>> + */
>>>> +static struct kernfs_open_node *kernfs_check_on_protected(struct kernfs_node *kn)
>>>> +{
>>>> +	return rcu_dereference_check(kn->attr.open,
>>>> +				      lockdep_is_held(&kernfs_open_file_mutex));
>>>> +}
>>>
>>> Maybe name this just kernfs_deref_on()?
>>>
>>
>> Sure. I can mention in the function comment that this should be used
>> only under kernfs_open_file_mutex.
> 
> and in the check condition, add the conditions that you need to make this
> not trigger warning when used in all the places that you wanna use it.
> 

Since ->attr.open is always accessed/modified under kernfs_open_file_mutex, I
have included this check in helper which should be used only while holding this
mutex. Do you mean that I should include some additional checks as well in the
below helper:

+/*
+ * Deref ->attr.open corresponding to @kn while holding open_file_mutex.
+ * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely
+ * safely accessed outside RCU read-side critical section, while holding
+ * the mutex.
+ */
+static struct kernfs_open_node *kernfs_deref_on(struct kernfs_node *kn)
+{
+       return rcu_dereference_protected(kn->attr.open,
+                               lockdep_is_held(&kernfs_open_file_mutex));
+}
+

>> +static struct kernfs_open_node *kernfs_deref_on_raw(struct
>> kernfs_open_file *of, struct kernfs_node *kn)
>>  {
>> -       return rcu_dereference_raw(kn->attr.open);
>> +       struct kernfs_open_node *on;
>> +
>> +       if (list_empty(&of->list))
>> +               return NULL;
>> +
>> +       on = rcu_dereference_raw(kn->attr.open);
>> +
>> +       if (list_empty(&on->files))
>> +               return NULL;
>> +       else
>> +               return on;
> 
> Put the above conditions in the rcu_dereference_check(). That's what it is
> for - describing the additional conditions that make the dereference safe.
> 

As mentioned earlier, I have included checking of @of->list in
rcu_dereference_check. I am not sure if we can include checking of on->files as
well because "on" itself is the dereferenced pointer value here. So I have kept
checking of on->files separate as shown in the earlier snippet of
kernfs_deref_on_check above.

Thanks
-- Imran