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:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
CC:     Jeff Layton <jlayton@kernel.org>,
        Dave Chinner <david@fromorbit.com>,
        Frank van der Linden <fllinden@amazon.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't use GFP_KERNEL from
 nfsd_getxattr()/nfsd_listxattr()
Thread-Topic: [PATCH] nfsd: don't use GFP_KERNEL from
 nfsd_getxattr()/nfsd_listxattr()
Thread-Index: AQHZb4qhR4jn7R6+a0evVV4Y3qmRcq8si0OAgAAQUgCAABjIgIAAToKAgADRqYCAAMVVAIAEEwGA
Date:   Wed, 19 Apr 2023 13:51:12 +0000
Message-ID: <A23409BB-9BA1-44E5-96A8-C080B417CCB5@oracle.com>
References: <72bf692e-bb6b-c1f2-d1ba-3205ab649b43@I-love.SAKURA.ne.jp>
 <4BC7955B-40E4-4A43-B2D1-2E9302E84337@oracle.com>
 <b014047a-4a70-b38f-c5bb-01bc3c53d6f2@I-love.SAKURA.ne.jp>
 <aee35d52ab19e7e95f69742be8329764db72cbf8.camel@kernel.org>
 <c310695e-4279-b1a7-5c2a-2771cc19aa66@I-love.SAKURA.ne.jp>
 <7246a80ae33244a4553bbc0ca9e771ce8143d97b.camel@kernel.org>
 <20230416233758.GD447837@dread.disaster.area>
In-Reply-To: <20230416233758.GD447837@dread.disaster.area>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?q8vVWfUkemkv+HObC0JBRx3eLinY0j0aXPFM7KrlIpZxtuNttuUCbvzY8Q3C?=
 =?us-ascii?Q?pSbuaRb2IwK9wLImNhpEBYwpeWe/iPWDl3vR0YNaKhEWMPpbxS3D/osYHl3I?=
 =?us-ascii?Q?Y1XsyvNACuEcPLuqcDOWg2/kI7k3BRSfzYgAycxnKWlH34D+4kzELX4/WodP?=
 =?us-ascii?Q?7IRLV2Uwi7DvoYsXK7xxCYULq21c0R+MofIqT4RCi+B9rxQb6yCwb1Cqou3Z?=
 =?us-ascii?Q?dwV9vtWl0WcsCYBiTyA6JApOT2+COBxjTuLZ+8tvyVRNF2tJ+Pf5qzVPcqfB?=
 =?us-ascii?Q?OzBIER7J/uMsLOUPYKz28bllzgv+YwFc45IwppHIRpD9tmUWDys5W19ahC3H?=
 =?us-ascii?Q?nNJqPjYIC7hY/8KAcTueuVzmP1lJF/AGPWLlCUIqc2/TkIKVFvuiaczu4BCZ?=
 =?us-ascii?Q?m829Tbv7JSaxPHBM+axyoryVnXBcnyrlOSQExUqK4kwMyPzqase2E2RxfuOx?=
 =?us-ascii?Q?JZHbAItmwxEJRO/+COkJN5KwGQ4yq2HNcVkyBa1BulfiqDEJ2FX0vXgRC612?=
 =?us-ascii?Q?BFE+JLMZ/iN33y9QZxb2TpLE19Sb++sRQlwgfv4pBGu8EQJ4FqfIvDPk0Unm?=
 =?us-ascii?Q?5yeZjyCgwm4uwVLve60J/iuvo3bsWlVOXbbwOgU8utHpCusUjnwcWZ+szAFW?=
 =?us-ascii?Q?mmp36PMsNmQ4IiTA6vZuR1HeWzileMPtlsWj9wEIs5B5UcWrhvpga/aNdfsA?=
 =?us-ascii?Q?B+U0zPSdI95x3kfPmoq6/pS4yDeU0z06LkDcEmE26UQKwIf1kFizPQofMSfj?=
 =?us-ascii?Q?dlwRnzBqSiBoQM7KpzOmew3JMaYIBLtIkWuLbjctE0jNyPK8/LtSdtpBIMgi?=
 =?us-ascii?Q?A47zIdw5lKPdt5TuTfddZecgGwDoSDEsIk7dzoexMXF3bjSESkKC1U46r7Bn?=
 =?us-ascii?Q?YoDiAwRauFoPluIMWr0tNDUdY0DuR7nd6ipJLsaaN76SYOWZLFxYVsXyM+U0?=
 =?us-ascii?Q?jP3Rk/wsgS0XnOQN0rhX615+M+FWptZSD1WW1O69FRPFxlbHXQKRr2htXiz4?=
 =?us-ascii?Q?tiSMTca3hq8Qb13xmBs4wxcZ0wBYi2bbAZxpjwRkxTE+gzDBjsENzP/6tNl+?=
 =?us-ascii?Q?k1yJMf6TU/d0wgWRQc24I0eYHnuAN47HlyiLNiywy9wdemnelIbYKQGABzkw?=
 =?us-ascii?Q?XOoufxyx1+p7OmnmdGo5EUQ2HddE8+VVk+MFzzxLlotvUgDqCl7yRJaty9BL?=
 =?us-ascii?Q?1gEt2pi7iF4noAaifdbOl6f8pL4EaeUZFbpheNt+VGlliZFqGNVFbU54FEF0?=
 =?us-ascii?Q?6gzp0ntkWmF0SdfUOMR83wccQ7BTbC0Kce9XS8dqGEPa7TOesGoSYRMsFJEZ?=
 =?us-ascii?Q?yMPg8b/SHhNQJzNtu8v3aWFDl7+HIue4eHnmXmcXIVVssiLjXjqB5r3T1Nki?=
 =?us-ascii?Q?uehwuQN12LAqFDb+DaFo2PGA0zNiTb2/80Bmaj9z8ZVhTW4a/KjzO26Rokbm?=
 =?us-ascii?Q?ajrnMD0pIT7kPdTHFGM8viDEuOYn5006+D10FeGZwayhpjwib0t5wbG62RlD?=
 =?us-ascii?Q?07y+MEcGckelCmDKQkZsLq3YB/MtqK9HIxpP488eydGvI05N4IQd3RBSkD6B?=
 =?us-ascii?Q?ppTgrlePe7HMbnRsy93fsZmyCKsEuaIfiwI8Ct2UgBMD3iXdC+R5BQJ2iZei?=
 =?us-ascii?Q?PQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3E6E10D6CAEF844393CEA00765E1AA2C@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: D3tXUm4PaxDlWuL7w3QiydxPeW6j3DVnP8Ovl+glstRtA0zQnN0QZPUqk7Kwf9ClrKQiusjj0njAxSztaRlTf7C+aE9AAH5UQPUisM4hgz6FaCfL9ycMSlRekIPTRtVv9Ftp6YHMvmBOBGATrPaKdALIkYH1Rajc7+J2Vm/m9YWGsSVHqqSu7HZiXI3Kh9GjbQYLn8zucMIPp45fe6JdnT4SEz+hj76sywh5uRLi+m+T3vi1iyKYUYqIIIfhl9VWwYQwpMWl25m+w7qU6RnJQQom2QFPFyya573kZKbU7Z+cYN5xqsTBa9mWU99UHgoTJ5JFMJqHV+6wLUU6S3mzI3u1nS8R2VwhL6IcQ1PDnNy3AnIaBS9U7rVhkRzVfKrBHTuFU//R/XvI99YlcQslpsy+MVKn5g8nEMjmh+YvcT839332RSNnMjwehmNl0SvPc6AsyzYiZQuEeiUCWGkNRqmgQhKDROpOXpKFQQpI1mkw6WjUXBjrGl+xGoxVqqTjM4XfV3SwSYebzeljul29Y9PnO/AJStJw3gwncJRFC8KIMsGoP7oLI4e/44hAPl8N1R+aSqcfd69ctcfIU6PkdEnBE0U7yIPEgo+Ir5PEdldLceweCK8VbBPyoS7c7pEUQNc4hj3m7qtbC60IonWsyGN11JrGa+p7SaHWw7EHTV+I02RKATBxAJyhXK/LqIeiWuTClP8VcXIBd9FZjea67yv7dYridd9199qlt+ny5gEyKYXpR5XCcKxnN30/fkDuMX0XvFHyhc1Bsvl97C6oEjkFLDPU/Lm5oqYUFfpWDBPrmXLDVmIViAixfddjdtXZIjhym+MG8yWMGe4jFyqfJwT17+HEz8BlikQdR8jfLoZzJpVKMii+4s9Kq4Foral23eiMJSbMJjyAWxeakBh2Dv1s46IWnyLiElOsk8AWEDw=
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: 8582868f-145c-4b9c-de7b-08db40dd2325
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Apr 2023 13:51:12.4130
 (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: 8xZNReyvdcPk7IABUkEuvt9kupwFIIxUZcyn/yBBwxfx85EcphLtkba+fXWQn7Nu8pDGyGP2SkXJfPn+JKWCbA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4368
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-19_08,2023-04-18_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=873 adultscore=0
 phishscore=0 malwarescore=0 spamscore=0 bulkscore=0 suspectscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2304190124
X-Proofpoint-GUID: SOb4JX87teHYIbmFikFhb3MZd0tF6ART
X-Proofpoint-ORIG-GUID: SOb4JX87teHYIbmFikFhb3MZd0tF6ART
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,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-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Apr 16, 2023, at 7:37 PM, Dave Chinner <david@fromorbit.com> wrote:
>=20
> On Sun, Apr 16, 2023 at 07:51:41AM -0400, Jeff Layton wrote:
>> On Sun, 2023-04-16 at 08:21 +0900, Tetsuo Handa wrote:
>>> On 2023/04/16 3:40, Jeff Layton wrote:
>>>> On Sun, 2023-04-16 at 02:11 +0900, Tetsuo Handa wrote:
>>>>> On 2023/04/16 1:13, Chuck Lever III wrote:
>>>>>>> On Apr 15, 2023, at 7:07 AM, Tetsuo Handa <penguin-kernel@I-love.SA=
KURA.ne.jp> wrote:
>>>>>>>=20
>>>>>>> Since GFP_KERNEL is GFP_NOFS | __GFP_FS, usage like GFP_KERNEL | GF=
P_NOFS
>>>>>>> does not make sense. Drop __GFP_FS flag in order to avoid deadlock.
>>>>>>=20
>>>>>> The server side threads run in process context. GFP_KERNEL
>>>>>> is safe to use here -- as Jeff said, this code is not in
>>>>>> the server's reclaim path. Plenty of other call sites in
>>>>>> the NFS server code use GFP_KERNEL.
>>>>>=20
>>>>> GFP_KERNEL memory allocation calls filesystem's shrinker functions
>>>>> because of __GFP_FS flag. My understanding is
>>>>>=20
>>>>>  Whether this code is in memory reclaim path or not is irrelevant.
>>>>>  Whether memory reclaim path might hold lock or not is relevant.
>>>>>=20
>>>>> . Therefore, question is, does nfsd hold i_rwsem during memory reclai=
m path?
>>>>>=20
>>>>=20
>>>> No. At the time of these allocations, the i_rwsem is not held.
>>>=20
>>> Excuse me? nfsd_getxattr()/nfsd_listxattr() _are_ holding i_rwsem
>>> via inode_lock_shared(inode) before kvmalloc(GFP_KERNEL | GFP_NOFS) all=
ocation.
>>> That's why
>>>=20
>>> /*
>>>  * We're holding i_rwsem - use GFP_NOFS.
>>>  */
>>>=20
>>> is explicitly there in nfsd_listxattr() side.
>=20
> You can do GFP_KERNEL allocations holding the i_rwsem just fine.
> All that it requires is the caller holds a reference to the inode,
> and at that point inode will should skip the given inode without
> every locking it.

This suggests that the fix is to replace "GFP_KERNEL | GFP_NOFS"
with "GFP_KERNEL" /and/ ensure those paths are holding an
appropriate inode reference.

I'd rather not weaken the retry semantics of the memory
allocation if we do not have to do so.


> Of course, lockdep can't handle the "referenced inode lock ->
> fsreclaim -> unreferenced inode lock" pattern at all. It throws out
> false positives when it detects this because it's not aware of the
> fact that reference counts prevent inode lock recursion based
> deadlocks in the vfs inode cache shrinker.
>=20
> If a custom, non-vfs shrinker is walking inodes that have no
> references and taking i_rwsem in a way that can block without first
> checking whether it is safe to lock the inode in a deadlock free
> manner, they are doing the wrong thing and the custom shrinker needs
> to be fixed.
>=20
>>>=20
>>> If memory reclaim path (directly or indirectly via locking dependency) =
involves
>>> inode_lock_shared(inode)/inode_lock(inode), it is not safe to use __GFP=
_FS flag.
>>>=20
>>=20
>> (cc'ing Frank V. who wrote this code and -fsdevel)
>>=20
>> I stand corrected! You're absolutely right that it's taking the i_rwsem
>> for read there. That seems pretty weird, actually. I don't believe we
>> need to hold the inode_lock to call vfs_getxattr or vfs_listxattr, and
>> certainly nothing else under there requires it.
>>=20
>> Frank, was there some reason you decided you needed the inode_lock
>> there? It looks like under the hood, the xattr code requires you to take
>> it for write in setxattr and removexattr, but you don't need it at all
>> in getxattr or listxattr. Go figure.
>=20
> IIRC, the filesytsem can't take the i_rwsem for get/listxattr
> because the lookup contexts may already hold the i_rwsem. I think
> this is largely a problem caused by LSMs (e.g. IMA) needing to
> access security xattrs in paths where the inode is already
> locked.
>=20
>> Longer term, I wonder what the inode_lock is protecting in setxattr and
>> removexattr operations, given that get and list don't require them?
>> These are always delegated to the filesystem driver -- there is no
>> generic xattr implementation.
>=20
> Serialising updates against each other. xattr modifications are
> supposed to be "atomic" from the perspective of the user - they see
> the entire old or the new xattr, never a partial value.
>=20
> FWIW, XFS uses it's internal metadata rwsem for access/update
> serialisation of xattrs because we can't rely on the i_rwsem for
> reliable serialisation. I'm guessing that most journalling
> filesystems do something similar.
>=20
> Cheers,
>=20
> Dave.
> --=20
> Dave Chinner
> david@fromorbit.com


--
Chuck Lever