Received-SPF: pass (google.com: domain of linux-kernel+bounces-204856-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 6 Jun 2024 14:07:19 -0400
From: "Liam R. Howlett" <Liam.Howlett@oracle.com>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>, linux-fsdevel@vger.kernel.org,
        brauner@kernel.org, viro@zeniv.linux.org.uk, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        gregkh@linuxfoundation.org, linux-mm@kvack.org, rppt@kernel.org
Subject: Re: [PATCH v3 4/9] fs/procfs: use per-VMA RCU-protected locking in
 PROCMAP_QUERY API
Message-ID: <rcjhp7lo4h42biynqf7t66vlcqemg7gucn2sbgmbdfycdpq3lr@isljix7btf7k>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, 
	Suren Baghdasaryan <surenb@google.com>, Andrii Nakryiko <andrii.nakryiko@gmail.com>, 
	Andrii Nakryiko <andrii@kernel.org>, linux-fsdevel@vger.kernel.org, brauner@kernel.org, 
	viro@zeniv.linux.org.uk, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, 
	bpf@vger.kernel.org, gregkh@linuxfoundation.org, linux-mm@kvack.org, rppt@kernel.org
References: <20240605002459.4091285-1-andrii@kernel.org>
 <20240605002459.4091285-5-andrii@kernel.org>
 <CAJuCfpFp38X-tbiRAqS36zXG_ho2wyoRas0hCFLo07pN1noSmg@mail.gmail.com>
 <CAEf4BzYv0Ys+NpMMuXBYEVwAaOow=oBgUhBwen7g=68_5qKznQ@mail.gmail.com>
 <ue44yftirugr6u4ewl5cvgatpqnheuho7rgax3jyg6ox5vruyq@7k6harvobd2q>
 <CAJuCfpEFpd-+DDr=EyA1gMKZcDZYpZN9pBuFczhVXrFSe11U_g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAJuCfpEFpd-+DDr=EyA1gMKZcDZYpZN9pBuFczhVXrFSe11U_g@mail.gmail.com>
User-Agent: NeoMutt/20231103
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?SkxZYVZpby9QS2NJaGFRQ09DL3hxRzY5MmVoNkFVQ3lWT2Q5bTN2MXV3a0g3?=
 =?utf-8?B?bW84bHEwc2k3SmJTcWpET0x1M00zT0J2bVlVVEE3TkhOWFQxT1NxQ0VlR0F5?=
 =?utf-8?B?aXhkUjdNZXY1QitCNUhUQmlpMVRkU3BSTW9VYzBpRnBEc3V0SGxmZjBqSE5S?=
 =?utf-8?B?cmErOThOOEVYdWFKc3VvV1h3bVhBamlIbmIvby8zN2N4clJRb01LM2JRbFhI?=
 =?utf-8?B?OHRqUlN5azJyZWVGK1Vpa2MyamFxbFhWaHlCdmp3MHpZbDZqT09hbzdjUUdF?=
 =?utf-8?B?djhGM1MwMTRmNDdHa0tEVW5CL2JmYTVxOHJCcmlETWI3RHAyNkk5RUJ0RlVu?=
 =?utf-8?B?K2ZSa3dueERRcVBQQUwwcDhUd2FzRURtUHMxRloxVGpNWDJZK2U1d1RIVDVI?=
 =?utf-8?B?RWk2UGxweStOa0tJSkVvd09FaGIxRDJDZUw5NUZBb0Ivc1IyMlZkNmpWNFZR?=
 =?utf-8?B?Z3N5QjMzRDUrcjBGNlZaSjVJVENGblNtZEJIVkl4NHZoMm5SUk9RaGFJMkpC?=
 =?utf-8?B?K0xJYWVOcHlXR1BKZmhrRk1lSWhVUXNUcjhzR2ZwdzRxRjRYN0N2UUdvMkds?=
 =?utf-8?B?NklTMXQ2U0lWYWlyZ05NYmVhSXM3c3dKK054dXhVRUl6T0Q2Y2ZUcmU5cnNY?=
 =?utf-8?B?N0tidmttUWxidUdRM2lwSWx6ejZ5M2Q1WDhQbG9PaktNS1ZWN2VKR2NuNlhY?=
 =?utf-8?B?cGUyYWtNa2lYOGVaVitIZnZOUnErVlViRlB2VVZMdTZxOGg5NEMzUm5YbFo5?=
 =?utf-8?B?SGpFRE0yaUZFY1BuY3JuTlVrMDBraUhucVlBQW8zNUNlWlZqdHhBbzJpeTFl?=
 =?utf-8?B?eEc3WmYwSDFueXNCSk1VS2NZVG1nVTY0dkZBU0xwb3ZXNGpLZ2ZMak9tSUZ0?=
 =?utf-8?B?bnFkbDF0alNUSWRyUGlFMGduaHArNE5ObUxiVFRPU25EdjR0eWtFM01XbVhE?=
 =?utf-8?B?NXNpMjhmcmNHanZ6b092T3grb2hONi9wS0F5OGlIcW5sNm1hRDdSZE15a1h4?=
 =?utf-8?B?eE93cmhicExxNXB0VXRJbllvYW9PbXVFM0c4cVExbUZOQ0t5bW1Ra3lMNTJZ?=
 =?utf-8?B?bmJqQzROdzY1R3M3Q1I5dHhhNzMvUzdObjVHMGZkS0xpMW1mZlhML2FUaTc5?=
 =?utf-8?B?SWp4blYxOElzbWdEUVF4cmNMckhrR05hWFhyU2xpZm82MTlLcVpMREl4cDRP?=
 =?utf-8?B?R213alpxeTNvOWdMai84L0k1NU1pWmFWUWZudXVYN2tNTWNzNHcvWENYZ0Jt?=
 =?utf-8?B?TVpBb3cvSjZiQWpVaFUyL0V6RGxwU3Eyc1pheU9pOVlVMjBWbEx4eERyUzFR?=
 =?utf-8?B?ZWM5MHdUZHJRZTF5a0xqazRGZnlHY2c1cTkyWjNielpocGVPSUw0Vys1YVBR?=
 =?utf-8?B?T1J2Sm9qZExxWTY1aWZqWUFvellNRmZmU3lPb0lGaWJ3U1d4M3MrWXA5NUZm?=
 =?utf-8?B?YnR1MFFsdkZUNHd6RWh0REliL1JKY1Z6SENXenF0NUovYVhiV3FmQVRGZzZI?=
 =?utf-8?B?b3c0c2Jpd25WaHF4WEQ5T0ZpRUVIaUFCSXNqbDcyRjZ6NzZBNnprRnljNWhm?=
 =?utf-8?B?a0xIYmtJVk5SRFdLQWlkdFB4aVE3eE9Fd3RYQ3RrajkvazBmdkpEZ1FsY280?=
 =?utf-8?B?c2tjblZxT2RBZlpHZS95dWZSSTBZMThMdzQydDNLMm03b2JjbytpZ0dKZVRQ?=
 =?utf-8?B?alBFUlJwR09SUW9jYWNHSWttcURBTnlRZkJsSWNSaXAwejJrcStJbzdBcU9p?=
 =?utf-8?B?OXErK3hhMXRqa0V1MGhkZGdocXZXRytKSEIxUXl0Rm1pcjNmdEdtVzRRaFc3?=
 =?utf-8?B?ajg5V05ERi9SVE9BZG5XbnhQcEt5L0wvWnMwd0xBU2JDQzllSjBiZVNKYnFu?=
 =?utf-8?B?OXQ1Q2ZKS0xuL056aU53RU9uV3huR2l6ckRrTXlPVklsOExFdjZHbWtCa3Vu?=
 =?utf-8?B?bWdqWklWNldRUXVhTkN5c1JHMFlRS2NjNk0weE4zWEIxemMzZmpMV3MyOXYr?=
 =?utf-8?B?ZlVnQlR6MU1ueGVGVXVKcWM1dEh3M0QzaERRQUJPdFRpRzc4WFozaEZublF2?=
 =?utf-8?B?ekVTTlZnVFZGbFEycHY5Ulc5a2tTK1ZxOGVsQ0RtWUE1cHVXUXlwNnNORXd5?=
 =?utf-8?Q?nQ2x5Vgwx25B+InCCVsH9JAGF?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	8fBK9BPAh4yOZg8TkARkV6/6Sl3f53JGlGn/I59xrmkbg4bKFiU/2TShmwdW1w7gBfXWRMpe6/24kKRgqOFsVv+FN9i+cyCd+UffgxrPtM1ttdgiIPNpxQ+NBdTNgc1aDIcIU8XOITabAUafKeP9knnPEN1ijrycqZe6an3Y47ZviROkwZiQL34bDDF4K/eg23SS7t91+JhSU/vabCYOMNZaZYWrRweIYmCze0tTWVX4lWfNKz3Tdg4/9KQUx107Jo0Af3xVUNGv6nmf7mFmEbe03oCkokfHBvGDiTbmv6SMLwF9jVYCu6GistQMy6VfhP27UEcLbJigr0KIVZlf3jV5Uuo5NI42taUieTI6XA4y2qSgJXWZvcOAsVAx3clbh0aDyJa1Op7bwi6xE6mnE1Md10QIM1S/DnFOG5SSxTIYjMuU2ytxVosj8UjDUEhg4voEBAsZyYpkyEQ7RUZJ3xF+kff6z0fdRl00wl5YCxDcHHsfMr47z+XiwtSzQu8cRjoB14Kb4f3QonMgtG7j4G7Bqvn5L4YHA0FI3E+dsME8UKMYw+0vWsJFISDUldZdCanboPXCz29dmvMqH3OPwpu1+g91NZcMAGuwFgGnur8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55d9ea30-7096-4b57-03cd-08dc86538317
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7933.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 18:07:22.1000
 (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: le+21pOVzFgyuAgetGezAyIkRiNITgJuZubIlS3mnW7RPa4++pA07661mDUNjH5QA/DXWZnbvEb3ySfHMq91fw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5669
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-06_14,2024-06-06_02,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 adultscore=0 spamscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2406060125
X-Proofpoint-GUID: oOkWR0A1ns1aBmR6v84apFdtMu6fXAPL
X-Proofpoint-ORIG-GUID: oOkWR0A1ns1aBmR6v84apFdtMu6fXAPL

* Suren Baghdasaryan <surenb@google.com> [240606 13:33]:
> On Thu, Jun 6, 2024 at 10:15=E2=80=AFAM Liam R. Howlett <Liam.Howlett@ora=
cle.com> wrote:
> >
> > * Andrii Nakryiko <andrii.nakryiko@gmail.com> [240606 12:52]:
> > > On Wed, Jun 5, 2024 at 4:16=E2=80=AFPM Suren Baghdasaryan <surenb@goo=
gle.com> wrote:
> > > >
> > > > On Tue, Jun 4, 2024 at 5:25=E2=80=AFPM Andrii Nakryiko <andrii@kern=
el.org> wrote:
> > > > >
> > > > > Attempt to use RCU-protected per-VMA lock when looking up request=
ed VMA
> > > > > as much as possible, only falling back to mmap_lock if per-VMA lo=
ck
> > > > > failed. This is done so that querying of VMAs doesn't interfere w=
ith
> > > > > other critical tasks, like page fault handling.
> > > > >
> > > > > This has been suggested by mm folks, and we make use of a newly a=
dded
> > > > > internal API that works like find_vma(), but tries to use per-VMA=
 lock.
> > > > >
> > > > > We have two sets of setup/query/teardown helper functions with di=
fferent
> > > > > implementations depending on availability of per-VMA lock (condit=
ioned
> > > > > on CONFIG_PER_VMA_LOCK) to abstract per-VMA lock subtleties.
> > > > >
> > > > > When per-VMA lock is available, lookup is done under RCU, attempt=
ing to
> > > > > take a per-VMA lock. If that fails, we fallback to mmap_lock, but=
 then
> > > > > proceed to unconditionally grab per-VMA lock again, dropping mmap=
_lock
> > > > > immediately. In this configuration mmap_lock is never helf for lo=
ng,
> > > > > minimizing disruptions while querying.
> > > > >
> > > > > When per-VMA lock is compiled out, we take mmap_lock once, query =
VMAs
> > > > > using find_vma() API, and then unlock mmap_lock at the very end o=
nce as
> > > > > well. In this setup we avoid locking/unlocking mmap_lock on every=
 looked
> > > > > up VMA (depending on query parameters we might need to iterate a =
few of
> > > > > them).
> > > > >
> > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > ---
> > > > >  fs/proc/task_mmu.c | 46 ++++++++++++++++++++++++++++++++++++++++=
++++++
> > > > >  1 file changed, 46 insertions(+)
> > > > >
> > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > > index 614fbe5d0667..140032ffc551 100644
> > > > > --- a/fs/proc/task_mmu.c
> > > > > +++ b/fs/proc/task_mmu.c
> > > > > @@ -388,6 +388,49 @@ static int pid_maps_open(struct inode *inode=
, struct file *file)
> > > > >                 PROCMAP_QUERY_VMA_FLAGS                         \
> > > > >  )
> > > > >
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +static int query_vma_setup(struct mm_struct *mm)
> > > > > +{
> > > > > +       /* in the presence of per-VMA lock we don't need any setu=
p/teardown */
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static void query_vma_teardown(struct mm_struct *mm, struct vm_a=
rea_struct *vma)
> > > > > +{
> > > > > +       /* in the presence of per-VMA lock we need to unlock vma,=
 if present */
> > > > > +       if (vma)
> > > > > +               vma_end_read(vma);
> > > > > +}
> > > > > +
> > > > > +static struct vm_area_struct *query_vma_find_by_addr(struct mm_s=
truct *mm, unsigned long addr)
> > > > > +{
> > > > > +       struct vm_area_struct *vma;
> > > > > +
> > > > > +       /* try to use less disruptive per-VMA lock */
> > > > > +       vma =3D find_and_lock_vma_rcu(mm, addr);
> > > > > +       if (IS_ERR(vma)) {
> > > > > +               /* failed to take per-VMA lock, fallback to mmap_=
lock */
> > > > > +               if (mmap_read_lock_killable(mm))
> > > > > +                       return ERR_PTR(-EINTR);
> > > > > +
> > > > > +               vma =3D find_vma(mm, addr);
> > > > > +               if (vma) {
> > > > > +                       /*
> > > > > +                        * We cannot use vma_start_read() as it m=
ay fail due to
> > > > > +                        * false locked (see comment in vma_start=
_read()). We
> > > > > +                        * can avoid that by directly locking vm_=
lock under
> > > > > +                        * mmap_lock, which guarantees that nobod=
y can lock the
> > > > > +                        * vma for write (vma_start_write()) unde=
r us.
> > > > > +                        */
> > > > > +                       down_read(&vma->vm_lock->lock);
> > > >
> > > > Hi Andrii,
> > > > The above pattern of locking VMA under mmap_lock and then dropping
> > > > mmap_lock is becoming more common. Matthew had an RFC proposal for =
an
> > > > API to do this here:
> > > > https://lore.kernel.org/all/ZivhG0yrbpFqORDw@casper.infradead.org/.=
 It
> > > > might be worth reviving that discussion.
> > >
> > > Sure, it would be nice to have generic and blessed primitives to use
> > > here. But the good news is that once this is all figured out by you m=
m
> > > folks, it should be easy to make use of those primitives here, right?
> > >
> > > >
> > > > > +               }
> > > > > +
> > > > > +               mmap_read_unlock(mm);
> > > >
> > > > Later on in your code you are calling get_vma_name() which might ca=
ll
> > > > anon_vma_name() to retrieve user-defined VMA name. After this patch
> > > > this operation will be done without holding mmap_lock, however per
> > > > https://elixir.bootlin.com/linux/latest/source/include/linux/mm_typ=
es.h#L582
> > > > this function has to be called with mmap_lock held for read. Indeed
> > > > with debug flags enabled you should hit this assertion:
> > > > https://elixir.bootlin.com/linux/latest/source/mm/madvise.c#L96.
> >
> > The documentation on the first link says to hold the lock or take a
> > reference, but then we assert the lock.  If you take a reference to the
> > anon vma name, then we will trigger the assert.  Either the
> > documentation needs changing or the assert is incorrect - or I'm missin=
g
> > something?
>=20
> I think the documentation is correct. It says that at the time of
> calling anon_vma_name() the mmap_lock should be locked (hence the
> assertion). Then the user can raise anon_vma_name refcount, drop
> mmap_lock and safely continue using anon_vma_name object. IOW this is
> fine:
>=20
> mmap_read_lock(vma->mm);
> anon_name =3D anon_vma_name(vma);
> anon_vma_name_get(anon_name);
> mmap_read_unlock(vma->mm);
> // keep using anon_name
> anon_vma_name_put(anon_name);
>=20

I consider that an optimistic view of what will happen, but okay.