From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Neil Brown <neilb@suse.de>,
        Trond Myklebust <trondmy@hammerspace.com>
CC:     "Paul E. McKenney" <paulmck@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2] NFSD: Fix RCU-related sparse splat
Thread-Topic: [PATCH v2] NFSD: Fix RCU-related sparse splat
Thread-Index: AQHX5XDFTrw4hMDSskyK33okWYrHZawcrx4AgAAMIoCAAAzJgA==
Date:   Wed, 1 Dec 2021 00:22:02 +0000
Message-ID: <2ABC02B1-CE76-422A-B64F-64B108B12C0B@oracle.com>
References: <163821156142.90770.4019362941494014139.stgit@bazille.1015granger.net>
 <20211130225250.GC641268@paulmck-ThinkPad-P17-Gen-1>
 <163831537509.26075.12859361728901873347@noble.neil.brown.name>
In-Reply-To: <163831537509.26075.12859361728901873347@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7KYrRB7o70n/oaiRmrRqRkpVZqb7wyvOmop3PS/8WuheCcmnHgPyhBGqKl29?=
 =?us-ascii?Q?EqxOXrW47Hu7p4iQV2lUBx6O86SKbv2GbnuyVrYCyohwzwGPfZr5XFBIIehc?=
 =?us-ascii?Q?Yehf4Vtr/t8LEA+Jvsr+oLxgNNYuY0qCclc7kAij5XfTDjb/d3LnghTgRGAs?=
 =?us-ascii?Q?JdTkaioIQv2U5JgIRaeV2IL9m7z/6V3zI2ZRp5DixiarMuBMva+DBzvHuLt7?=
 =?us-ascii?Q?HMKP/JcV9hKwbzq/u2GKTEvRiPqRXIWfA/5hnTc6WBZNwV3Zt/gU7Dih7fze?=
 =?us-ascii?Q?/N/df8NmcM7n3V1PvkJ5nmQyMt1Cqr9i8rnWGt8O7NL2Z0DFSV25IPBpOsQ6?=
 =?us-ascii?Q?HK+toPjPjVPMv+6+r0XInlXH/2PBzCdpyFRn/2hn3iLYGDvSToif5mbrz9ht?=
 =?us-ascii?Q?4lJM3aGpBxc5B1C6PuamW09XGDc0Rwg7UYDnStm3ELtEm2VbQfi5ZmyUUBPB?=
 =?us-ascii?Q?tfglLp8fFsUTtnWHEI9jE/V+mDHoQlH+H012mHxKDFt2/E0YSd2e6nZt5wCk?=
 =?us-ascii?Q?FZYMVBhMqTOOAozFtDF15sU7xo9pyy+ZJ8655lljp1Vi2EjGZKkTFBJ/0qHX?=
 =?us-ascii?Q?qgS6o7NCItFrENlYZf+pK1UtDLilP884ETZwh8xxM4hZUe4uvxsU3iKvakPV?=
 =?us-ascii?Q?7jH7Apzzd6TC3y2otgbBtR+n+1xNtH4qJZDLpfVWjuJ2jRrZZr0wrRHjjUsY?=
 =?us-ascii?Q?1BKnzSuPkitvHf4eFmtuj5RY0uZBo9Ex1srGhZJQOUJ4Q2ZiCW5U6K32iB63?=
 =?us-ascii?Q?SLXGLlm6EMjhUzHG/iez4vaR5iFWq1bvq4APPSHZMJdW6BwRsibdlluChgu/?=
 =?us-ascii?Q?V/ntIFXbo4eFMYVoUOfdR97Inp9iQErQU5GIn/NN3JwZgAjJLIax23fjFmmw?=
 =?us-ascii?Q?l80nt7vZTQKF3Xtxmw5TX2jjQMn46fSvOgXq/fWbKsi63yn0Yi/vB5qqnYJc?=
 =?us-ascii?Q?HuDMCw1AXUJUdHQBYYtvkE73yFy/4wcNzlgfukmP/CnXYCdgHXy1uscAm9Qk?=
 =?us-ascii?Q?ZpK/JFVtr8FBKGKqKHCcoa3n3/4D0BRPl35Od/TedrnzbNkc+OlnRAq3c4ql?=
 =?us-ascii?Q?UTXqxkYDqbwWKpoXUWj8+JOEKXsyPOXIazGESOB4t/7CTS7gn2guQHlVarQF?=
 =?us-ascii?Q?DwrAiBgcVNzFxzi0ZCMD8ItaSEoi3rULAAlSSkSWKE/+0Q7El4G2UaQiCM18?=
 =?us-ascii?Q?y4ZMVZeUgb41pjQ4MtCRZT/NRYqwRnFgOzVjkM9kEW837o2e50Kx13kbxU3G?=
 =?us-ascii?Q?IlSJGGTF9EOo9DhiU+bVTY93Wx9YWKnLpWLmranJvv11p9w7a0txw+eHHCKH?=
 =?us-ascii?Q?mMStNRDLCJc3biJW8L3nV6J418Ufw4MboE/c6dJAvVhiQJTvmki5tK1RSoKI?=
 =?us-ascii?Q?VOeiv6sA5Se4cJz0uTsibcaaW0/rZfEQbL6AiytF7PTVDvCduthxpDzjplVE?=
 =?us-ascii?Q?3Ri7QRk98j6CG6Az8lZoyVoq5k5l2wKvDRV75egBVaB0HRL/MP0msWAVL4Gv?=
 =?us-ascii?Q?PDHN07TV+joI8oq9i/ZrwpbaIvRxP2Q+KCB03fgD01YOlzX00eWBqFRCePvX?=
 =?us-ascii?Q?VriHjLfR7MRyPPHhwarIGctsXayjVUZDBRVAcFz4O45mHIW4Fq8vwaDZREA4?=
 =?us-ascii?Q?ncSfFkC6qHLuuSGEzh4cuJk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C1155926C9A18141B1E559C86207BD41@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b1c6e67f-b274-4c34-9631-08d9b46098dc
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Dec 2021 00:22:02.3282
 (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: btuMNv8/+o1GKrlxSTzp+hyzKPFiSh7LDaKn1cBt0eWYAEZroRsZohAxagchqS2GA5qKV3V6M5KgZDsHHCntgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2693
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10184 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0 mlxscore=0
 suspectscore=0 mlxlogscore=999 spamscore=0 phishscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112010000
X-Proofpoint-ORIG-GUID: lu53wJbRI2hZIJuttHkj5xnOoJu5Yjy1
X-Proofpoint-GUID: lu53wJbRI2hZIJuttHkj5xnOoJu5Yjy1
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Nov 30, 2021, at 6:36 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> On Wed, 01 Dec 2021, paulmck@kernel.org wrote:
>> On Mon, Nov 29, 2021 at 01:46:07PM -0500, Chuck Lever wrote:
>>> To address this error:
>>>=20
>>>  CC [M]  fs/nfsd/filecache.o
>>>  CHECK   /home/cel/src/linux/linux/fs/nfsd/filecache.c
>>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9: error: incompatibl=
e types in comparison expression (different address spaces):
>>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9:    struct net [nod=
eref] __rcu *
>>> /home/cel/src/linux/linux/fs/nfsd/filecache.c:772:9:    struct net *
>>>=20
>>> The "net" field in struct nfsd_fcache_disposal is not annotated as
>>> requiring an RCU assignment, so replace the macro that includes an
>>> invocation of rcu_check_sparse() with an equivalent that does not.
>>>=20
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>=20
>> From an RCU perspective:
>>=20
>> Acked-by: Paul E. McKenney <paulmck@kernel.org>
>>=20
>> But it would be good to get someone more familiar with the code to
>> look at this.
>=20
> There is a global list of 'struct nfsd_fcache_disposal', potentially one
> for each network namespace.  Each contains a '*net' which is effectively
> an opaque lookup key.  It is never dereferenced - only used to find the
> nfsd_fcache_disposal which matches a given 'struct net *'.
>=20
> The lookup happens under rcu_read_lock().  A 'struct net' is freed after
> a grace period (see cleanup_net() in net_namespace.c) so to ensure the
> lookup doesn't find a false positive - caused by a 'struct net' being
> deallocated and reallocated, it is sufficient to clear the ->net pointer
> while the the net is known to still be active.  At most, a write-barrier
> might be justified.  i.e. smp_store_release(l->net, NULL);

By way of further explanation:

The Documentation/ for RCU (ie, "RCU for Dummies") suggests that
rcu_assign_pointer() is adequate for preventing undesirable
compiler optimizations or CPU load/store reordering.

rcu_assign_pointer() uses WRITE_ONCE for constants like NULL and
smp_store_release() when assigning variable values. I copied the
use of WRITE_ONCE() from rcu_assign_pointer(). So I expect exactly
zero change in behavior or compiled code... (but I should have
checked the generated object to verify that is the case).


> However ... the list of nfsd_fcache_disposal seems unnecessary.
> nfsd has a 'struct nfsd_net' which parallels any interesting 'struct
> net' using the net_generic() framework.
> If the nfs_fcache_disposal were linked from the nfsd_net, there would be
> no need for the list, no need to record the ->net, and not need for the
> code in question here.
> The nfsd_fcache_disposal is destroyed (nfsd_file_cache_shutdown_new())
> before the nfsd_net is destroyed, so there are no lifetime issues with
> keeping the separate list.

Sure. If it makes sense to use nfsd_net instead of a separate data
structure, then that can be made to happen. I would like to hear
from Trond regarding why he felt a separate data structure was
necessary for fcache_disposal.


--
Chuck Lever