| Message ID | e73da1304a88b6a8a11907045117cccf4c2b8374.1648046642.git.quic_charante@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: madvise: return exact bytes advised with process_madvise under error | expand |
On Wed 23-03-22 20:54:09, Charan Teja Kalla wrote: > This reverts commit 08095d6310a7 ("mm: madvise: skip unmapped vma holes > passed to process_madvise") as process_madvise() fails to return exact > processed bytes at other cases too. As an example: if the > process_madvise() hits mlocked pages after processing some initial bytes > passed in [start, end), it just returns EINVAL though some bytes are > processed. Thus making an exception only for ENOMEM is partially fixing > the problem of returning the proper advised bytes. > > Thus revert this patch and return proper bytes advised, if there any, > for all the error types in the following patch. I do agree with the revert. I am not sure the above really is a proper justification though. 08095d6310a7 was changing one (arguably) dubious semantic by another one without a proper justification and wider consensus which I would expect from a patch which changes an existing semantic. Not to mention it being marked for stable tree. But let's not nit pick on that now. Let's send this revert ASAP and use some more time to discuss the semantic and whether any change is really required. > Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/madvise.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 39b712f..0d8fd17 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1433,16 +1433,9 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, > > while (iov_iter_count(&iter)) { > iovec = iov_iter_iovec(&iter); > - /* > - * do_madvise returns ENOMEM if unmapped holes are present > - * in the passed VMA. process_madvise() is expected to skip > - * unmapped holes passed to it in the 'struct iovec' list > - * and not fail because of them. Thus treat -ENOMEM return > - * from do_madvise as valid and continue processing. > - */ > ret = do_madvise(mm, (unsigned long)iovec.iov_base, > iovec.iov_len, behavior); > - if (ret < 0 && ret != -ENOMEM) > + if (ret < 0) > break; > iov_iter_advance(&iter, iovec.iov_len); > } > -- > 2.7.4
Thanks Michal. On 3/24/2022 6:18 PM, Michal Hocko wrote: > On Wed 23-03-22 20:54:09, Charan Teja Kalla wrote: >> This reverts commit 08095d6310a7 ("mm: madvise: skip unmapped vma holes >> passed to process_madvise") as process_madvise() fails to return exact >> processed bytes at other cases too. As an example: if the >> process_madvise() hits mlocked pages after processing some initial bytes >> passed in [start, end), it just returns EINVAL though some bytes are >> processed. Thus making an exception only for ENOMEM is partially fixing >> the problem of returning the proper advised bytes. >> >> Thus revert this patch and return proper bytes advised, if there any, >> for all the error types in the following patch. > > I do agree with the revert. I am not sure the above really is a proper > justification though. 08095d6310a7 was changing one (arguably) dubious > semantic by another one without a proper justification and wider > consensus which I would expect from a patch which changes an existing > semantic. Not to mention it being marked for stable tree. Thanks for pointing this out. Since 08095d6310a7 is marked for stable tree, doing the same for this change. Cc: <stable@vger.kernel.org> # 5.10+ > > But let's not nit pick on that now. Let's send this revert ASAP and use > some more time to discuss the semantic and whether any change is really > required. > >> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> > > Acked-by: Michal Hocko <mhocko@suse.com> > Thanks for the quick ack. >> --- >> mm/madvise.c | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/mm/madvise.c b/mm/madvise.c >> index 39b712f..0d8fd17 100644 >> --- a/mm/madvise.c >> +++ b/mm/madvise.c >> @@ -1433,16 +1433,9 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, >> >> while (iov_iter_count(&iter)) { >> iovec = iov_iter_iovec(&iter); >> - /* >> - * do_madvise returns ENOMEM if unmapped holes are present >> - * in the passed VMA. process_madvise() is expected to skip >> - * unmapped holes passed to it in the 'struct iovec' list >> - * and not fail because of them. Thus treat -ENOMEM return >> - * from do_madvise as valid and continue processing. >> - */ >> ret = do_madvise(mm, (unsigned long)iovec.iov_base, >> iovec.iov_len, behavior); >> - if (ret < 0 && ret != -ENOMEM) >> + if (ret < 0) >> break; >> iov_iter_advance(&iter, iovec.iov_len); >> } >> -- >> 2.7.4 >
diff --git a/mm/madvise.c b/mm/madvise.c index 39b712f..0d8fd17 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1433,16 +1433,9 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec, while (iov_iter_count(&iter)) { iovec = iov_iter_iovec(&iter); - /* - * do_madvise returns ENOMEM if unmapped holes are present - * in the passed VMA. process_madvise() is expected to skip - * unmapped holes passed to it in the 'struct iovec' list - * and not fail because of them. Thus treat -ENOMEM return - * from do_madvise as valid and continue processing. - */ ret = do_madvise(mm, (unsigned long)iovec.iov_base, iovec.iov_len, behavior); - if (ret < 0 && ret != -ENOMEM) + if (ret < 0) break; iov_iter_advance(&iter, iovec.iov_len); }
This reverts commit 08095d6310a7 ("mm: madvise: skip unmapped vma holes passed to process_madvise") as process_madvise() fails to return exact processed bytes at other cases too. As an example: if the process_madvise() hits mlocked pages after processing some initial bytes passed in [start, end), it just returns EINVAL though some bytes are processed. Thus making an exception only for ENOMEM is partially fixing the problem of returning the proper advised bytes. Thus revert this patch and return proper bytes advised, if there any, for all the error types in the following patch. Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com> --- mm/madvise.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)