| Message ID | 20240621181224.3881179-1-audra@redhat.com |
|---|---|
| State | Accepted |
| Commit | 1723f04caacb32cadc4e063725d836a0c4450694 |
| Headers | show |
| Series | [v2,1/3] Fix userfaultfd_api to return EINVAL as expected | expand |
On Fri, Jun 21, 2024 at 02:12:22PM -0400, Audra Mitchell wrote: > Currently if we request a feature that is not set in the Kernel > config we fail silently and return all the available features. However, > the man page indicates we should return an EINVAL. > > We need to fix this issue since we can end up with a Kernel warning > should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on > a kernel with the config not set with this feature. > > [ 200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660 > [ 200.820738] Modules linked in: > [ 200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8 > [ 200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022 > [ 200.885052] RIP: 0010:zap_pte_range+0x43d/0x660 > > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API") > Signed-off-by: Audra Mitchell <audra@redhat.com> Please prefix the subject with "mm/uffd:" or "mm/userfault:" if there's a repost. Acked-by: Peter Xu <peterx@redhat.com> Thanks,
On Fri, Jun 21, 2024 at 02:12:23PM -0400, Audra Mitchell wrote: > Now that we have updated userfaultfd_api to correctly return > EIVAL when a feature is requested but not available, let's fix > the uffd-stress test to only set the UFFD_FEATURE_WP_UNPOPULATED > feature when the config is set. In addition, still run the test if > the CONFIG_PTE_MARKER_UFFD_WP is not set, just dont use the corresponding > UFFD_FEATURE_WP_UNPOPULATED feature. > > Signed-off-by: Audra Mitchell <audra@redhat.com> Acked-by: Peter Xu <peterx@redhat.com>
On Fri, 21 Jun 2024 14:12:22 -0400 Audra Mitchell <audra@redhat.com> wrote: > Currently if we request a feature that is not set in the Kernel > config we fail silently and return all the available features. However, > the man page indicates we should return an EINVAL. > > We need to fix this issue since we can end up with a Kernel warning > should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on > a kernel with the config not set with this feature. > > [ 200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660 > [ 200.820738] Modules linked in: > [ 200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8 > [ 200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022 > [ 200.885052] RIP: 0010:zap_pte_range+0x43d/0x660 > > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API") A userspace-triggerable WARN is bad. I added cc:stable to this.
On Fri, Jun 21, 2024 at 06:03:30PM -0700, Andrew Morton wrote: > On Fri, 21 Jun 2024 14:12:22 -0400 Audra Mitchell <audra@redhat.com> wrote: > > > Currently if we request a feature that is not set in the Kernel > > config we fail silently and return all the available features. However, > > the man page indicates we should return an EINVAL. > > > > We need to fix this issue since we can end up with a Kernel warning > > should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on > > a kernel with the config not set with this feature. > > > > [ 200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660 > > [ 200.820738] Modules linked in: > > [ 200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8 > > [ 200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022 > > [ 200.885052] RIP: 0010:zap_pte_range+0x43d/0x660 > > > > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API") > > A userspace-triggerable WARN is bad. I added cc:stable to this. Andrew, Note that this change may fix a WARN, but it may also start to break userspace which might be worse if it happens, IMHO. I forgot to mention that here, but only mentioned that in v1, and from that POV not copying stable seems the right thing. https://lore.kernel.org/all/ZjuIEH8TW2tWcqXQ@x1n/ In summary: I think we can stick with Fixes on e06f1e1dd499, but we don't copy stable. The major reason we don't copy stable here is not only about complexity of such backport, but also that there can be apps trying to pass in unsupported bits (even if the kernel didn't support it) but keep using MISSING mode only, then we shouldn't fail them easily after a stable upgrade. Just smells dangerous to backport. I'm not sure what's the best solution for this. A sweet spot might be that we start to fix it in new kernels only but not stable ones. Thanks,
On Sun, 23 Jun 2024 11:26:37 -0400 Peter Xu <peterx@redhat.com> wrote: > > > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API") > > > > A userspace-triggerable WARN is bad. I added cc:stable to this. > > Andrew, > > Note that this change may fix a WARN, but it may also start to break > userspace which might be worse if it happens, IMHO. I forgot to mention > that here, but only mentioned that in v1, and from that POV not copying > stable seems the right thing. > > https://lore.kernel.org/all/ZjuIEH8TW2tWcqXQ@x1n/ > > In summary: I think we can stick with Fixes on e06f1e1dd499, but we > don't copy stable. The major reason we don't copy stable here is > not only about complexity of such backport, but also that there can > be apps trying to pass in unsupported bits (even if the kernel > didn't support it) but keep using MISSING mode only, then we > shouldn't fail them easily after a stable upgrade. Just smells > dangerous to backport. OK. And I'll move it into the 6.11-rc1 queue, for the next merge window.
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index eee7320ab0b0..17e409ceaa33 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -2057,7 +2057,7 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx, goto out; features = uffdio_api.features; ret = -EINVAL; - if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES)) + if (uffdio_api.api != UFFD_API) goto err_out; ret = -EPERM; if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE)) @@ -2081,6 +2081,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx, uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED; uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC; #endif + + ret = -EINVAL; + if (features & ~uffdio_api.features) + goto err_out; + uffdio_api.ioctls = UFFD_API_IOCTLS; ret = -EFAULT; if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
Currently if we request a feature that is not set in the Kernel config we fail silently and return all the available features. However, the man page indicates we should return an EINVAL. We need to fix this issue since we can end up with a Kernel warning should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on a kernel with the config not set with this feature. [ 200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660 [ 200.820738] Modules linked in: [ 200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8 [ 200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022 [ 200.885052] RIP: 0010:zap_pte_range+0x43d/0x660 Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API") Signed-off-by: Audra Mitchell <audra@redhat.com> --- fs/userfaultfd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)