Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Wed, 25 Oct 2023 14:07:38 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Ingo Molnar <mingo@redhat.com>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: nfsd_copy_write_verifier: wrong usage of read_seqbegin_or_lock()
Message-ID: <ZTlZan240vG8HG/B@tissot.1015granger.net>
References: <20231025163006.GA8279@redhat.com>
 <ZTlJmuDpGE+U3pEF@tissot.1015granger.net>
 <20231025175435.GC29779@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231025175435.GC29779@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?h5MUC9fRPGvs1fVWuAlqx+2V33SvjrXGcsXV1V9tehDHMiAz4WhIBmRaJePe?=
 =?us-ascii?Q?D+8u09UpJG3SnONPsmYwYMWoM7hDzm2EaFEFpxFoAdT9R0u57+FncOALY2Qr?=
 =?us-ascii?Q?moAh5JwJSsDJwulLIvaicAi83kFjDgRGmMIuXe/LxD8/oPbwFJBMK1Obl0EP?=
 =?us-ascii?Q?bgiKAHfv0Qmq2x1gFRlgSHv+XMei+i/uU4w6EK/8f6p3DgDUcIezWuG9fNGS?=
 =?us-ascii?Q?xhJjv2XdMkUkgiN71zxAX486EEy06iZE7HGL/wvw2UCFd7E4QMU75T2Hpbco?=
 =?us-ascii?Q?mmmQdlOmkluw8s6E6VICyrxkuh+poOM32ff52fvrxZla85sNIzpCc2WHIfmk?=
 =?us-ascii?Q?VFmFJJUGcwxZ0QlN4bzJZjsMQVE9XMIB81fpfMIwp5yAnKI2gKVwUiNoBfo0?=
 =?us-ascii?Q?FMZsV5ngXnrOumPaUMrxBQsNCgay2m+28XiyNWra/j6zcCHenXI8OtOBMWgZ?=
 =?us-ascii?Q?ugPpcwKxY8Pt2CHx3q86gWf7MxioQWQDVcNU7IN4VgdQZOnRNUVeBM6rCBYx?=
 =?us-ascii?Q?jP6Tcazc0vQhnSlWrIPsq7r/s1HE2tUPB/ROQ3jj+ORWMhFGFKvGR4Wk4ecK?=
 =?us-ascii?Q?mbgFBKzI/CnzPHowqheE+Y+BKsFQkbLVnDQl9q51LK2cfY6c8uAB0lY8XIId?=
 =?us-ascii?Q?2uzzr5gpOH50yKipR9v4ZhyEvTq1is9uKJMGzmnLEoTtGD+gxSAtL4wm6HCT?=
 =?us-ascii?Q?4CdIjUJj5b5WGSqf1626YH9K2J8wf6bw5FW2aDUyg/ZDSloHDBphGbfe6rAo?=
 =?us-ascii?Q?s+ZBRBoM6ML5j/cFcb4pI4DIR6lhgHZi/dKQlBcycU2bOBj8EUTUioKz9w70?=
 =?us-ascii?Q?nW4NJfZjZvVZLtGNZz7ECnr9H0TO8k9/OblJLQs0aDJamiFBnxrZi44LAIzH?=
 =?us-ascii?Q?xmwaD07nduC+ET1d5IWF+/0n658ZmFzYsaUMVFHMuoSYcIFDTcpbmavMUbEw?=
 =?us-ascii?Q?ltRow5pOMdY3q8c0+bD0kKp65XmifuOM/ZvXDRNdGQ6Y15uhka8ahanaeeAh?=
 =?us-ascii?Q?nGRY/5OJ8SrKxMoj4sLqbZLQjIpQQPzEpZ34nQcIuxg3xYGNymVUXeDMhjp2?=
 =?us-ascii?Q?QB1jtRm9ekSuUDEObApowAur9L63gXU39K7UKZMOHVKfCQKhbFkSueSPyEOG?=
 =?us-ascii?Q?v92DPitXgxk6t45kLm+FZ5IecJ4e+AoWasrID6AKUX9MrVpdnd4TSbXIuG1l?=
 =?us-ascii?Q?oZpNY9s1mjaW8Llkf3pFBeohnSslef3uTrxRzYOOLxKAOiMIe4eTwx2vFmXf?=
 =?us-ascii?Q?z4dWs8t32Yygxw2ydvtT/54KFI4YCywwjHb0pRB1RfwAZBeB+aWEc6rwJvZP?=
 =?us-ascii?Q?u/XgTNdx6AkJwxMwEZFj5JO4H2lfFlZ/2E9UV/SlRAG/WE0EiuDUo6oQymf8?=
 =?us-ascii?Q?6EIkBBjkO+0pE5p8LJacJgrdXj7dOcCYB3driLfYYXiJVmIiG6qn5/9+LzNr?=
 =?us-ascii?Q?ETdELmf1REwzzfYh69EeNv99JK5dAyHkIDX9xWCINva0/U5BMg21u0Wupd/s?=
 =?us-ascii?Q?uaQj9cKEnDNwcwPrGAX2yK6PQjsf+0IM/GILRP8LQPAB8yPpC0xvbqsla6uC?=
 =?us-ascii?Q?8fwHH4vUrBabsLZDAH2I+IQKTc/KwzTSh+oUkqdgJJI4X286nQ9XL8OMiExY?=
 =?us-ascii?Q?eQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 1vN236EmXeR9UtA2jdbtTjlAYs7LX+udqVdpLsJ63D6Pz9fZjLY2HlLG+KSw6hZLQecX+Ez8WrWMdnLcl+9+oTU/d6ExweL3BApGVK1pSbOhXu9bAJiCtkzr2ggUTkcQj9wDpOdthMbgUy1gbg7m/t63yCLNIWNTylWS4b02p6cMJ0x14d2eBZUoZu9O1PLrFkvLqIo0oA/8OsWnI860frSchgR0swfTW7ewtpbuDTSieFUDh3qsvZEGjzJzZYu9T/8hfOymQiDIVUJKvzZ2dlrVWcozk8fCzyyvpdxV7hKl5Ji4orTBXvAG2l2KVo2Uh941bnwAzDGPsv1jlkshumdXZUIPNnOhZ2MHhxa87ug3PqvJ3TTx7TW8nQVmNQrTlZCS89VSYW+y2xLfaZOMDKg8wJPra1VEaGDUOkZhhC66OJLJ0ewzSkySvHva+rCFfg526P5rcuvf6A/crkJWk/hbrLcHe1YTuHN6BC0l2DPTn9E8PSAHRvfbPrSocBwIeMWG7cnqp4V1lt3RXX4CBhMrWLoPUNVWW85i/ItmwOYKnU/YYK7qEghK7fX9lgwDml3SiajVOIoHDeCUXIu5C1wnMMnFTTAwW/38K8qipFtRrW0zBGT70TtOIgt3WC1unj7L95gKzPvGO9HET9rOsxuuQl5EpSd+r40/xZQQYnjTpz8pzf78tRetHCbPfiWAm/6G+/0zbkEG9akIsZmOsjIlMR6/+4Bc8n1ooi7dnV+JQMZrNyGToh7/k4JZWtGpUAO6sxKCbntRr7nwotsUu0x2fIPN/qzBz7G3ukvEls27iZQ5pelGGhP/3SKmUJyZURT9P8KEs2yUJKim2l9NSxlCZSZ7CadvG7VA8uookiOjG7wZyEg4ZAx4+cY9s0rWBxkMVd6fUQyYwGORH81xKQ==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55202029-b3ef-4415-ce18-08dbd585483e
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2023 18:07:42.3987
 (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: /olJW03R1wTvUt1Ao+Pm/H75XeJq2886klXOZj9Zl4RNW9e+PhX12aYuJdkr/FzIfT2+1Z2hcoJmhxYDOz/Q4g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB6715
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-25_07,2023-10-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0 suspectscore=0
 phishscore=0 malwarescore=0 mlxscore=0 mlxlogscore=999 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001
 definitions=main-2310250157
X-Proofpoint-GUID: VVj5Fj2TLGPE3RzooJcZxlDbomXMZiHV
X-Proofpoint-ORIG-GUID: VVj5Fj2TLGPE3RzooJcZxlDbomXMZiHV
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 25 Oct 2023 11:09:14 -0700 (PDT)

On Wed, Oct 25, 2023 at 07:54:36PM +0200, Oleg Nesterov wrote:
> On 10/25, Chuck Lever wrote:
> >
> > > Another question is why we can't simply turn nn->writeverf into seqcount_t.
> > > I guess we can't because nfsd_reset_write_verifier() needs spin_lock() to
> > > serialise with itself, right?
> >
> > "reset" is supposed to be very rare operation. Using a lock in that
> > case is probably quite acceptable, as long as reading the verifier
> > is wait-free and guaranteed to be untorn.
> >
> > But a seqcount_t is only 32 bits.
> 
> Again, I don't understand you.
> 
> Once again, we can turn writeverf into seqcount_t, see the patch below.

The patch below does not turn "writeverf" into a seqcount_t, it
turns "writeverf_lock" into a seqcount_t. "writeverf" is an 8-byte
field, that's why I said "seqcount_t is only 32 bits" -- that is
not an adequate replacement for the 8-byte "writeverf" field.

Your original proposal made no sense. But I see now what you
would like to change.

I'm not familiar enough with these primitives to have a strong
opinion. What do you think would be the benefit?


> But this way nfsd_reset_write_verifier() can race with itself, no?

> Oleg
> ---
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ec49b200b797..3e2adf3eb15f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -110,7 +110,7 @@ struct nfsd_net {
>  	bool nfsd_net_up;
>  	bool lockd_up;
>  
> -	seqlock_t writeverf_lock;
> +	seqcount_t writeverf_lock;
>  	unsigned char writeverf[8];
>  
>  	/*
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..6320491018f8 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1523,7 +1523,7 @@ static __net_init int nfsd_net_init(struct net *net)
>  	nn->nfsd4_minorversions = NULL;
>  	nfsd4_init_leases_net(nn);
>  	get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key));
> -	seqlock_init(&nn->writeverf_lock);
> +	seqcount_init(&nn->writeverf_lock);
>  
>  	return 0;
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index c7af1095f6b5..fc4e31411508 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -359,13 +359,12 @@ static bool nfsd_needs_lockd(struct nfsd_net *nn)
>   */
>  void nfsd_copy_write_verifier(__be32 verf[2], struct nfsd_net *nn)
>  {
> -	int seq = 0;
> +	int seq;
>  
>  	do {
> -		read_seqbegin_or_lock(&nn->writeverf_lock, &seq);
> +		seq = read_seqcount_begin(&nn->writeverf_lock);
>  		memcpy(verf, nn->writeverf, sizeof(nn->writeverf));
> -	} while (need_seqretry(&nn->writeverf_lock, seq));
> -	done_seqretry(&nn->writeverf_lock, seq);
> +	} while (read_seqcount_retry(&nn->writeverf_lock, seq));
>  }
>  
>  static void nfsd_reset_write_verifier_locked(struct nfsd_net *nn)
> @@ -397,9 +396,9 @@ static void nfsd_reset_write_verifier_locked(struct nfsd_net *nn)
>   */
>  void nfsd_reset_write_verifier(struct nfsd_net *nn)
>  {
> -	write_seqlock(&nn->writeverf_lock);
> +	write_seqcount_begin(&nn->writeverf_lock);
>  	nfsd_reset_write_verifier_locked(nn);
> -	write_sequnlock(&nn->writeverf_lock);
> +	write_seqcount_end(&nn->writeverf_lock);
>  }
>  
>  /*
> 

-- 
Chuck Lever