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: <6d1c5a49-fc16-5fc7-e527-8cb91ec0d25c@oracle.com>
Date:   Mon, 13 Jun 2022 12:36:12 +1000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.10.0
Subject: Re: [PATCH RESEND v4 1/4] 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: <20220602063907.136569-1-imran.f.khan@oracle.com>
 <20220602063907.136569-2-imran.f.khan@oracle.com>
 <YqYpTShTDLvAp4vx@slm.duckdns.org>
From:   Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <YqYpTShTDLvAp4vx@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?bXl5TmdoM2JxNnlBeFIxdHh2RE5kbVZOa3QyS0ZnOE5PcGVkR2Zwb2JXcmxN?=
 =?utf-8?B?b20zN0lKdGhiZDZtSjBycjI2elB6OGkza0txUVZyNFhkdUR5TlNyN2ZJOXNw?=
 =?utf-8?B?R2ZTM2V6L3Q2S29iYlJBZVp2MTIzVkFvTDBQcUk3eWhZbGxSa1ZiYitFN0lt?=
 =?utf-8?B?Z0ZNbFRSVTdpVnd2V0xpYmRFNzRtT2RSVGg2MG5SQ2RSZ0Yvc2NpaHU0bExy?=
 =?utf-8?B?QzFnQjhnTU80Sjk3RzNkN0NVMXVibHA2SGxNR1p4WjBJTTE5VkF1eGJNSzBB?=
 =?utf-8?B?NXVWcXVYUzdXckR2cHF2TTVpK0NYSUN4OG4xdGJZVEV1SzhiZ0NCZ1pUWHdn?=
 =?utf-8?B?UGRQd25WcnFEdFB3eWgvOGIrdnRZZWJER3RtS1oxY21TbzIwT0hhZ3ZaWkhM?=
 =?utf-8?B?eXhBM0RCcll5U0F2a2k0a01DSlpGOC9hTHpIeFVtWWtDKzBOeklNa1VkM1NL?=
 =?utf-8?B?QjMzUXMzdXVJTWVDcXQ3RUwyUzBDYUZtREdZaFR3aGV5cElBajFkNGxFZkhQ?=
 =?utf-8?B?aWxSd05qUmZ4ZU9TcWc1L0FZRVZiNWVmVGYwdU9wNlA4RlNBMzlVS2I5SnVt?=
 =?utf-8?B?UTdtRk43aEd0Ym05dUNSOXpDaGJ6ZUoyWmllR3BJOGpxdDZoQ1o1QUZudHFF?=
 =?utf-8?B?VW5BaGxHaGNUSUhOT2xSMmVJTDdvaHNsK1lSbDlxdUpLeHpDYlE5YVFEb0Jl?=
 =?utf-8?B?V2ZYMTVrbVRoeFppVklZajdCcUF1K0NyUkl6V21WYXJkZXhwbGFlQlUxMS9F?=
 =?utf-8?B?ZTVaTnAyRy9HYmpFRWpYL1g0OTlwbSt6UkJWcWUyb1hVcW1rbE9ZQS96SlNW?=
 =?utf-8?B?RkdKeUxTZGdDYjBlbVl0Q3ovTjRvUTh2MGp3cFBYSXVRWS9HWlU1VXhMZkp0?=
 =?utf-8?B?Mnh3S1hZU1d0LzRSQmtIL1V2cmFMSG1PeWphRktWOXRBY0VOQmpLUG1FdUtG?=
 =?utf-8?B?bkFidjB1Mk1NeWtERlZDVWJBMmhMQy8wMHRHYmlkREFUaXZNd24rVzM2Y1lt?=
 =?utf-8?B?RmJLWndmSWVOY1dxZStqeVZIRzZ0S3dtaUxoSzlTa0UvVkx6bXlmdFZHZTM5?=
 =?utf-8?B?Vk02YmlTL3Jac1VRc0Jxa1dzSzVhTWdURE9rWjNJbzdWdzlaQVpVZ3NNN0Nh?=
 =?utf-8?B?OExMUGJwdURCNGtxQnpBWk5wOC9vK01GeHNLelRxMUc1QXFzS1ViN3I2UnNQ?=
 =?utf-8?B?YkZzdmJlcGh4YlZTWDN1YnJ1Vnl0enFxUCtWVjdzaFJJdlNNN1Jham40eEls?=
 =?utf-8?B?RzhpK0paMzYrS1ZtQzRpU0xIR05uY0YrWSsxWnpBdnllUjZDemJWZ2tqS3M5?=
 =?utf-8?B?NkZlYTBLblYzUWc1cHVLWWEwbWd0RVN5MU9wMmMzaWRpdkRnMy9XbkpMOWEr?=
 =?utf-8?B?ZmRHak1WRFRVMTBPQ1luNC80eTZmcW1YbXVhejJxNm5Rd1NVSi8xckVMZkFQ?=
 =?utf-8?B?R015UUxyVHpwVUUwdHRNeUpxWVQ5VVlrNmVhS3hpbVNYeFR2TUx2aEdtMCt5?=
 =?utf-8?B?R1RLd1VYU3ZKd1N3dFFhdDJMOURJeTA5UW1lWWRpM1VlTHFvZWIrQ1pvU3F5?=
 =?utf-8?B?Tk44M2taSzZBY0NhODRsaUZhZ3F5VmNwOGN3M1MrZXc5YnE2dlJiZ29vbnBB?=
 =?utf-8?B?RTBhTDBqdFVXajI4bGZra2FJcUNtM1M3REhOVkpUNy9UZFdWSExlYUZHOS9J?=
 =?utf-8?B?VGpCNW9XeG1UQzBzbnQrWWUxSDFRYU9jNlo2ZURmb0pmMS9OeGJOVXFvOERw?=
 =?utf-8?B?MzNiUnRBemtHYWl0dFh6UDAxbjB4Nk52bG9zbzEzY3Z6TXBjTTBWa29NTEFS?=
 =?utf-8?B?RUdCWi9rZHpMRVJNcjJCbHIrdm1KQUpCT3NqSmFPd1ZaaThQUUl1c0tpS1Bo?=
 =?utf-8?B?aXRxVnZEUXNQb1I3THZvMTdwSGVUQVZnQ0oyVTlQTTlTaTB3QmtVczBpM0I5?=
 =?utf-8?B?WHNuNkE5cFRGK3p4aHVEZTZKTlVkVFZUR2V0c09uWFlNWkxoUktKKzY0RmZG?=
 =?utf-8?B?enBvckNHWWJ1RDM2SlRFMXl1bVpneDcrQTA1THJZZHZjSlE0SGFQT2dCTVNi?=
 =?utf-8?B?cUJUYUJiU0JaaDA1enhLV0NyY0RqZTEzZW9IVVdvRkpreXhOMjJGRHI1SnE2?=
 =?utf-8?B?QVhCTWZrYzdDaU5FeFJoaitRWitqZWl5NHYydDg3NG43U1Y4RTZYMXYrUjNl?=
 =?utf-8?B?K29pMG9xS0ROV0NvNVlwMDRXeHVmZHRHNVQvai9TRU5XWXhrS01LWjRnWkNp?=
 =?utf-8?B?SktSdmsveEMyRXI5NElIWHhSK1h6UFdKT2FoQllHeDdJa2taZzh0RysrZks4?=
 =?utf-8?B?WDRNdzRFckR2dXpCVVRDVkhNc0VUWDRKYVVYb2ZtNkl4c3Z0OElvcGYxT1gr?=
 =?utf-8?Q?phFLQtFoJ9oSH7IU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 866cafa1-ead7-48ef-1362-08da4ce57f12
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2022 02:36:19.2428
 (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: nX08OQ5Oo9Uw9D9Umh8zZ6MCR73N9ZJxHWVmzn0c9NtX60ieV1Mbfa5dmbztc7Knk9wjB9se1Ct7Be4eYYupCA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1858
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.874
 definitions=2022-06-13_01:2022-06-09,2022-06-13 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 bulkscore=0 spamscore=0
 phishscore=0 malwarescore=0 mlxscore=0 mlxlogscore=999 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2206130009
X-Proofpoint-GUID: mrHU5giwhPF6IAcVgYkkSThLOVG-5BbH
X-Proofpoint-ORIG-GUID: mrHU5giwhPF6IAcVgYkkSThLOVG-5BbH
X-Spam-Status: No, score=-4.0 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_H2,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,

On 13/6/22 3:58 am, Tejun Heo wrote:
> Hello,
> 
> Sorry about the long delay.
>
No problem.

[...]
>> +/**
>> + * kernfs_check_open_node_protected - Get kernfs_open_node corresponding to @kn
>> + *
>> + * @kn: target kernfs_node.
>> + *
>> + * Fetch and return ->attr.open of @kn when caller(reader) holds
>> + * kernfs_open_file_mutex.
>> + *
>> + * Update of ->attr.open happens under kernfs_open_file_mutex. So as long as
>> + * the current reader (caller) is holding this mutex, updaters will not be
>> + * able to change ->attr.open and this means that we can safely deref
>> + * ->attr.open outside RCU read-side critical section.
>> + *
>> + * This should ONLY be used by readers of ->attr.open and caller needs to make
>> + * sure that kernfs_open_file_mutex is held.
>> + */
>> +static struct kernfs_open_node *
>> +kernfs_check_open_node_protected(struct kernfs_node *kn)
>> +{
>> +	return rcu_dereference_check(kn->attr.open,
>> +				      lockdep_is_held(&kernfs_open_file_mutex));
>> +}
> 
> I don't understand why the above is necessary. Whether you're a reader or
> writer, you can deref the pointer w/ _protected as long as you're holding
> the lock, right?

As per [1], we should use rcu_dereference_check() for the reader side when we
are holding the lock.

> If I'm mistaken and somehow a reader needs to use a
> deref_check, I don't see a reason for this to be a separate accessor. Why
> not just merge the condition into the kernfs_deref_open_node()?
>

I have not merged lockdep_is_held(&kernfs_open_file_mutex) in
kernfs_deref_open_node() because not all users of kernfs_deref_open_node() (i.e.
kernfs_seq_show(), kernfs_file_read_iter(), kernfs_generic_poll()) can guarantee
that kernfs_open_file_mutex has been acquired. So at the moment we have 2
helpers around rcu_dereference_check, kernfs_check_open_node_protected() for
readers under kernfs_open_file_mutex and kernfs_deref_open_node for readers that
rely on existence of kernfs_open_file by making sure that ->list is not empty.

Could you please let me know if this sounds reasonable to you ?

[1]:
https://www.kernel.org/doc/html/v5.6/RCU/rcu_dereference.html#which-member-of-the-rcu-dereference-family-should-you-use


Thanks again for your review.
  -- Imran