Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Liam Howlett <liam.howlett@oracle.com>
To:     Jann Horn <jannh@google.com>
CC:     Luigi Rizzo <lrizzo@google.com>, Jason Gunthorpe <jgg@ziepe.ca>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Rientjes <rientjes@google.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*()
Thread-Topic: [PATCH] Add mmap_assert_locked() annotations to find_vma*()
Thread-Index: AQHXhjmoYtMFsOr1JUGtL1OdK4dvaath9waAgABfMgCAABX5gIABBTqAgAAvfAA=
Date:   Wed, 4 Aug 2021 17:32:27 +0000
Message-ID: <20210804173220.nzn7kka3oyzcsxks@revolver>
References: <20210731175341.3458608-1-lrizzo@google.com>
 <20210803160803.GG543798@ziepe.ca>
 <CAMOZA0JKjRFUHbs3zc4kiGcuXxR0arCN=oPZZsLCa4qHvRrH_A@mail.gmail.com>
 <20210803230725.ao3i2emejyyor36n@revolver>
 <CAG48ez2TEP0hsRjLACVmRppMEk6Z9aREcGL498EKhdBBXSRsoA@mail.gmail.com>
In-Reply-To: <CAG48ez2TEP0hsRjLACVmRppMEk6Z9aREcGL498EKhdBBXSRsoA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?2xM5lnYla60CgTIeRjSVLG/4v3ehji59h5c/xBcFZeTB5gH3M6BgXtzeiNkg?=
 =?us-ascii?Q?xXzLOGJzXYISe4QdndHhEh9ctUdrPNQ7J9+at8iTOe5ZR1TgxlIvpYpI26F1?=
 =?us-ascii?Q?aEH3MZygDGvdkhwL139c9L57uZYGqu05qkRMpfxl/u3F5WgR4p4TP66N8HkK?=
 =?us-ascii?Q?+cjFuuOVp67fvQgKvWOosjFVTiA4oJ818LjJYeaXDlijD9giTqcUCk9pf2uU?=
 =?us-ascii?Q?RGMdeSLe8q3/h7jPgpoxQv8eoU2QhXblDiaOazA0W1fCGIvhizk2CEVjnUfs?=
 =?us-ascii?Q?AQZpNNHy+rRiwY5HzSIvw+aIR4+Z3zmTvIW5Xs27JOkq7iBUl7WMbBZodxHY?=
 =?us-ascii?Q?rzLaQPqDiEbWWA+1HJO+eW5UGoWRTpPZEpv/HWbQfMPzv0xKGEva22rPmSoa?=
 =?us-ascii?Q?0JVQxetLCCi7lcUa2gIVNZ6vLv1RxaNNJ5F1G70ptf9hYHWBziZsrTpvzYEw?=
 =?us-ascii?Q?vxnYeYWQjHrkR7F1rlVHT9WhES+U25rTZI+wMgx70ylxiIXcZ8ApVTZiGOkb?=
 =?us-ascii?Q?YP9KnltKWm99HgEuqZzMyEZAuiH/BDVZkDAS31S98agvGBgGfcA+/uYhRB29?=
 =?us-ascii?Q?gxPJtitnp4FCUXoEmAIGjFtj2sXNlvo/Br7lGmJuDHyeqoGwCfPgCVCCnnCh?=
 =?us-ascii?Q?BxAb4FAfqqLN2Wsxiye5BJEqJJwqY2v6jOil9PlgIlaxX3X6abXEkloWxYWF?=
 =?us-ascii?Q?6o7Vc0xiNbuugKqgde/qjhNnQpto1hNFWZP8eBUesHOXN1GTYUkgSfQckPO6?=
 =?us-ascii?Q?cOz9/d5DSNBZ3X2ytnuXL0nC3TEt1+nRabRa+N+CBOWZFwE4LEn4VoFjfjCv?=
 =?us-ascii?Q?kjdrmwc88GHxGYzaxwpqDyCQWqUMpVtMCFjIHIn2IWCwpP9Dp7TaAQCKO2BM?=
 =?us-ascii?Q?J4VtrZCmaiRtGg6ei/cyBwKVdHcXatJHWOc73edwrSlq2hMKLSd6ga7ZmGvY?=
 =?us-ascii?Q?Zr0NpLHepr7qKvczDc62bTyEkjnCSq+UlIUi5EKutO52leWqRpyWpy0W2Ixc?=
 =?us-ascii?Q?LT0h/wj7Qtwn5B50BIu9el2NR8ZI0XHv9hgc26BzJBQfQMgPVd6QlCQSOnmY?=
 =?us-ascii?Q?IsQPpbau1TMl2BuzohW2L4asKCp6f6PvXBr21q6dYdwQDysnV45XPiOA3xaE?=
 =?us-ascii?Q?ggXWr6hscHRj024KI2Fk3wHjjny9M/GTZF4qjKPn/j5R/n9YZARN9ZVP3LRi?=
 =?us-ascii?Q?Fb1cHLq/nWoQ0+JKD2Mes6hDtXJKQro3qvqcxKNDMx7cQweILZrQmE2bB7sR?=
 =?us-ascii?Q?CcHX4z0WtWfEaKS0bwL6SbSZt9iv4tb/G4gpTRLEBOvYjvb04owRzd3J1tUf?=
 =?us-ascii?Q?ZJv1YmkO/mS/Cz8bNHK8NVQf?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1A842C2DBA51774892E0479779389892@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: DM6PR10MB4380.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 06a6040d-f6da-41a0-405b-08d9576dd480
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Aug 2021 17:32:27.7046
 (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: Xh8ejyXkFO+ZGOs6EbGCTz1gE+Jz1y0XRbpEKMzBqPZKzP+bV/jnubok1mOQx88lPtUAgyWRiVcLDXE9k80A0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3659
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10066 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 adultscore=0
 suspectscore=0 mlxlogscore=999 phishscore=0 spamscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2107140000
 definitions=main-2108040101
X-Proofpoint-GUID: TM830Sg2Yi9LO4W3YN-ygSrenZBVYSrS
X-Proofpoint-ORIG-GUID: TM830Sg2Yi9LO4W3YN-ygSrenZBVYSrS
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Jann Horn <jannh@google.com> [210804 10:42]:
> On Wed, Aug 4, 2021 at 1:07 AM Liam Howlett <liam.howlett@oracle.com> wro=
te:
> > * Luigi Rizzo <lrizzo@google.com> [210803 17:49]:
> > > On Tue, Aug 3, 2021 at 6:08 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Sat, Jul 31, 2021 at 10:53:41AM -0700, Luigi Rizzo wrote:
> > > > > find_vma() and variants need protection when used.
> > > > > This patch adds mmap_assert_lock() calls in the functions.
> > > > >
> > > > > To make sure the invariant is satisfied, we also need to add a
> > > > > mmap_read_loc() around the get_user_pages_remote() call in
> > > > > get_arg_page(). The lock is not strictly necessary because the mm
> > > > > has been newly created, but the extra cost is limited because
> > > > > the same mutex was also acquired shortly before in __bprm_mm_init=
(),
> > > > > so it is hot and uncontended.
> > > > >
> > > > > Signed-off-by: Luigi Rizzo <lrizzo@google.com>
> > > > >  fs/exec.c | 2 ++
> > > > >  mm/mmap.c | 2 ++
> > > > >  2 files changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > > index 38f63451b928..ac7603e985b4 100644
> > > > > +++ b/fs/exec.c
> > > > > @@ -217,8 +217,10 @@ static struct page *get_arg_page(struct linu=
x_binprm *bprm, unsigned long pos,
> > > > >        * We are doing an exec().  'current' is the process
> > > > >        * doing the exec and bprm->mm is the new process's mm.
> > > > >        */
> > > > > +     mmap_read_lock(bprm->mm);
> > > > >       ret =3D get_user_pages_remote(bprm->mm, pos, 1, gup_flags,
> > > > >                       &page, NULL, NULL);
> > > > > +     mmap_read_unlock(bprm->mm);
> > > > >       if (ret <=3D 0)
> > > > >               return NULL;
> > > >
> > > > Wasn't Jann Horn working on something like this too?
> > > >
> > > > https://lore.kernel.org/linux-mm/20201016225713.1971256-1-jannh@goo=
gle.com/
> > > >
> > > > IIRC it was very tricky here, are you sure it is OK to obtain this =
lock
> > > > here?
> > >
> > > I cannot comment on Jann's patch series but no other thread knows
> > > about this mm at this point in the code so the lock is definitely
> > > safe to acquire (shortly before there was also a write lock acquired
> > > on the same mm, in the same conditions).
> >
> > If there is no other code that knows about this mm, then does one need
> > the lock at all?  Is this just to satisfy the new check you added?
> >
> > If you want to make this change, I would suggest writing it in a way to
> > ensure the call to expand_downwards() in the same function also holds
> > the lock.  I believe this is technically required as well?  What do you
> > think?
>=20
> The call to expand_downwards() takes a VMA pointer as argument, and
> the mmap lock is the only thing that normally prevents concurrent
> freeing of VMA structs. Taking a lock there would be of limited utility -=
 either
> the lock is not necessary because nobody else can access the MM, or
> the lock is insufficient because someone could have freed the VMA
> pointer before the lock was taken. So I think that taking a lock
> around the expand_downwards() call would just be obfuscating things,
> unless you specifically want to prevent concurrent *reads* while
> concurrent *writes* are impossible.

Good point on the VMA being passed in, that certainly points to your
previous patch being a better approach.  That resolves my questions
around the patch.

>=20
> Since I haven't sent a new version of my old series for almost a year,
> I think it'd be fine to take Luigi's patch for now, and undo it at a
> later point when/if we want to actually use proper locking here
> because we're worried about concurrent access to the MM.=