Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Attila Kovacs <attila.kovacs@cfa.harvard.edu>
CC:     libtirpc List <libtirpc-devel@lists.sourceforge.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: Thread safe client destruction
Thread-Topic: Thread safe client destruction
Thread-Index: AQHYnTF6fzXlFwD9KEiv5zmVGW3BSq2JKdKA
Date:   Thu, 21 Jul 2022 18:45:54 +0000
Message-ID: <4D490A86-E873-40E7-A30E-9189B127C65C@oracle.com>
References: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu>
In-Reply-To: <53af8ec6-4ece-09b2-9499-d46d0fdfaa15@cfa.harvard.edu>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QvRuGHYD6S4VtFhj7zUZT4p7nmyvcgAZZqsBfDU+JhMmqX2t5AHaFC34jotA?=
 =?us-ascii?Q?KAYLYx4GYUlBs14XTpevoIKGYH78xm02j3Rbo9S9BPoQau9dCxFWblGVd12w?=
 =?us-ascii?Q?2B9ZK9G8xaiJzRuTIcgCgArgvAKrFdpVLZ1sw3fXsfktlEjVdx1eWdhYqWDm?=
 =?us-ascii?Q?zaUxi0jcA3TQ8dGerReOoYzfC6DZSdwgEeRgqBW48kOwq74pj+Np1ivaFwKP?=
 =?us-ascii?Q?QAStbYToh2E9FTbWSRbiVO6bUUtqr1aZBviX88K+Aw6D8lQfKRzXU6T9UtHc?=
 =?us-ascii?Q?wITB60YYnhokuOThKwkzNGqXUc+M6YY4uBHDppOAtytUIIJ+bjNBYsAQxuma?=
 =?us-ascii?Q?m9CPHG0eNGYViZCTE0GmDEUSpTmbfy2jmSwnVdJW5XhGHGlBp3T+crVqkKH8?=
 =?us-ascii?Q?0LVXn6i4VISX5KHovEArw3gTywkBzgkOcARF1Pd49HOP5kskobFXFbSAVSPx?=
 =?us-ascii?Q?MMOo8w1Mp8WqMEeqFnKz3DMO9KPLgiH0qIDLKqBCibvuPwzxS9M5mfJW9OO7?=
 =?us-ascii?Q?HC37xdY3JL21bmb6JCVotZv13DjYP3JjLOs2iXISuB2ELQ6JFSW6VXtOvDOz?=
 =?us-ascii?Q?Qhls4CdnXnf0x1ciqI+8sVeYNA54PtTTUXFBs7aHTHTMd/cXgjG1hXSphK5M?=
 =?us-ascii?Q?BLLePCk0geC4YnOBL55PMdCTRj6101JVSHd1qquc+CjOlQHah2f/DnWLrErI?=
 =?us-ascii?Q?uQLEhlID7FKdr9s/sJoXb8EXjp38pzYNaSv6WNFjrVZ8OzNjhZDTfyFyvHRj?=
 =?us-ascii?Q?k2rbPt4FZ2ia3JkSEmHKtDBEfujC7F9fi75dnrSqX9SeQ+P4RjYWu8NwzCYI?=
 =?us-ascii?Q?kj4gzc+yEyA02+mkxjeuUEwHxq6QkF31OYeysYtgqyfuST4jMm8MEU4ge3Ey?=
 =?us-ascii?Q?yK+dSWkw/QvP02tiXV5O40Ew0VGLpUjBNt8B9OSuu6KLbEVINlnCxW7LIB6B?=
 =?us-ascii?Q?HMA03VHpeJkpZAp58JRY/+DWs2gvi3rtm+yGWfcEvdIoQct31ws6Hb+sPHwC?=
 =?us-ascii?Q?sQUiSqhAsJrIOMW5jr0napUYRhyjB34jZ/eetzCS7faPY0JP6c/bs2yoeL4Q?=
 =?us-ascii?Q?li+gOHx2NpECOcRuon10gajqb62grt8rxcKmPAHC9lME9u56sMSkc3OA6s5N?=
 =?us-ascii?Q?GD/CLlqXV8UN/voeJDdkPF6OFS9Mt+qtqA0p6FNiH1y1geKCfq+S5syCyINA?=
 =?us-ascii?Q?M7s7UDF3WlSAGFEs8gU7nZMwkbQaZganxEcgDxGciIwqTmwCJneNHk8/mnso?=
 =?us-ascii?Q?cnqb4y9STo+SSGik1RZ204i+dfpHEngtWMoALWW7wokED2vWusu9lHRg4ND2?=
 =?us-ascii?Q?1rTa7JJkspvlRzRV0IVuxKvXtkxVJUWtz6hKAJQ2aUg3klmKjTHlP2Xmby2n?=
 =?us-ascii?Q?nStuKrjgXvzIHdsf5PWeaXQrWsT+YFNvp+e8rdWHF0BHYZw9h5Uay+jC4zbF?=
 =?us-ascii?Q?tHl+wZeFBAyf6hXd1J+tzXdBa2lmiOckgBpwIpa0G0DZsdpuAUWM758mk1Rz?=
 =?us-ascii?Q?LEbhguRY/VRMTum45K2oKPEOkUzbTplpJ2DZkJB7A2uzSfqFDi7jp7Xyh7Gr?=
 =?us-ascii?Q?n1IJaSvFET9fPWAYyx29H3wZ+vxSPxjhs2RHvapTQkepmAmLhvAWuMnpQjKv?=
 =?us-ascii?Q?rg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C5FFCF6AC4B3904E8754440A347F20C5@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: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ee052a7b-0222-4438-057f-08da6b493e1b
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jul 2022 18:45:54.4776
 (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: uTCPoclgDRPvhLw9vP82oKrV0EFtLUDcpSXkrZ76Hi53v3B3lGRQlaZtVgIUfENPIlf7wrNAoCq2A17D+i4J7A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB3790
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-21_26,2022-07-21_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 mlxlogscore=999 malwarescore=0 adultscore=0 mlxscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2206140000 definitions=main-2207210075
X-Proofpoint-GUID: JGSTIxNWGkb2BnaYjVMzZMFcDiakQCYY
X-Proofpoint-ORIG-GUID: JGSTIxNWGkb2BnaYjVMzZMFcDiakQCYY
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,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-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Jul 21, 2022, at 2:41 PM, Attila Kovacs <attila.kovacs@cfa.harvard.edu=
> wrote:
>=20
> Hi again,
>=20
>=20
> I found yet more potential MT flaws in clnt_dg.c and clnt_vg.c.

IIRC libtirpc is not MT-enabled because it was forked from the
Solaris libtirpc before MT-enablement was completed. I could
be remembering incorrectly.

Therefore I think we would need some unit tests along with the
improvements you propose. There's a test suite somewhere, but
someone else will need to provide details.

(and also, please inline your patch submissions rather than
attaching them, to make it easier for us to review. thanks!)


> 1. In clnt_dg.c in clnt_dg_freeres(), cu_fd_lock->active is set to TRUE, =
with no corresponding clearing when the operation (*xdr_res() call) is comp=
leted. This would leave other waiting operations blocked indefinitely, effe=
ctively deadlocked. For comparison, clnt_vd_freeres() in clnt_vc.c does not=
 set the active state to TRUE. I believe the vc behavior is correct, while =
the dg behavior is a bug.
>=20
> 2. If clnt_dg_destroy() or clnt_vc_destroy() is awoken with other blocked=
 operations pending (such as clnt_*_call(), clnt_*_control(), or clnt_*_fre=
eres()) but no active operation currently being executed, then the client g=
ets destroyed. Then, as the other blocked operations get subsequently awoke=
n, they will try operate on an invalid client handle, potentially causing u=
npredictable behavior and stack corruption.
>=20
> The proposed fix is to introduce a simple mutexed counting variable into =
the client lock structure, which keeps track of the number of pending block=
ing operations on the client. This way, clnt_*_destroy() can check if there=
 are any operations pending on a client, and keep waiting until all pending=
 operations are completed, before the client is destroyed safely and its re=
sources are freed.
>=20
> Attached is a patch with the above fixes.
>=20
> -- A.<clnt_mt_safe_destroy.patch>

--
Chuck Lever