| Message ID | x49tuknmosl.fsf@segfault.boston.devel.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pat: pass correct address to sanitize_phys | expand |
Ping? This patch fixes a real problem seen in the field. Can I get some review? Thanks! Jeff Jeff Moyer <jmoyer@redhat.com> writes: > memtype_reserve takes an address range of the form [start, end). It > then passes the start and end addresses to sanitize_phys, which is meant > to operate on the inclusive addresses. If end falls at the end of the > physical address space, sanitize_phys will return 0. This can result in > drivers failing to load: > > [ 10.000087] mpt3sas_cm0: unable to map adapter memory! or resource not found > [ 10.000334] mpt3sas_cm0: failure at drivers/scsi/mpt3sas/mpt3sas_scsih.c:10597/_scsih_probe()! > > Fix this by passing the inclusive end address to sanitize_phys. > > Fixes: 510ee090abc3 ("x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses") > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > -- > It might be worth adding a comment, here. If there are any suggestions > on what a sane wording would be, I'm all ears. > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > index 3112ca7786ed..482557905294 100644 > --- a/arch/x86/mm/pat/memtype.c > +++ b/arch/x86/mm/pat/memtype.c > @@ -583,7 +583,7 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type, > int err = 0; > > start = sanitize_phys(start); > - end = sanitize_phys(end); > + end = sanitize_phys(end - 1) + 1; > if (start >= end) { > WARN(1, "%s failed: [mem %#010Lx-%#010Lx], req %s\n", __func__, > start, end - 1, cattr_name(req_type));
On 21.07.21 21:48, Jeff Moyer wrote: > memtype_reserve takes an address range of the form [start, end). It > then passes the start and end addresses to sanitize_phys, which is meant > to operate on the inclusive addresses. If end falls at the end of the > physical address space, sanitize_phys will return 0. This can result in > drivers failing to load: > > [ 10.000087] mpt3sas_cm0: unable to map adapter memory! or resource not found > [ 10.000334] mpt3sas_cm0: failure at drivers/scsi/mpt3sas/mpt3sas_scsih.c:10597/_scsih_probe()! > > Fix this by passing the inclusive end address to sanitize_phys. > > Fixes: 510ee090abc3 ("x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses") > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > -- > It might be worth adding a comment, here. If there are any suggestions > on what a sane wording would be, I'm all ears. > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > index 3112ca7786ed..482557905294 100644 > --- a/arch/x86/mm/pat/memtype.c > +++ b/arch/x86/mm/pat/memtype.c > @@ -583,7 +583,7 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type, > int err = 0; > > start = sanitize_phys(start); > - end = sanitize_phys(end); > + end = sanitize_phys(end - 1) + 1; > if (start >= end) { > WARN(1, "%s failed: [mem %#010Lx-%#010Lx], req %s\n", __func__, > start, end - 1, cattr_name(req_type)); > > LGTM Reviewed-by: David Hildenbrand <david@redhat.com>
Jeff, On Wed, Jul 21 2021 at 15:48, Jeff Moyer wrote: Please write function names with brackets, i.e. sanitize_phys(). > memtype_reserve takes an address range of the form [start, end). It [start, end] > then passes the start and end addresses to sanitize_phys, which is meant > to operate on the inclusive addresses. If end falls at the end of the > physical address space, sanitize_phys will return 0. This can result in > drivers failing to load: > > [ 10.000087] mpt3sas_cm0: unable to map adapter memory! or resource not found > [ 10.000334] mpt3sas_cm0: failure at drivers/scsi/mpt3sas/mpt3sas_scsih.c:10597/_scsih_probe()! Doesn't this trigger the WARN() right below that offending line? > Fix this by passing the inclusive end address to sanitize_phys. > > Fixes: 510ee090abc3 ("x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses") > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > -- > It might be worth adding a comment, here. If there are any suggestions > on what a sane wording would be, I'm all ears. See below. > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > index 3112ca7786ed..482557905294 100644 > --- a/arch/x86/mm/pat/memtype.c > +++ b/arch/x86/mm/pat/memtype.c > @@ -583,7 +583,7 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type, > int err = 0; > > start = sanitize_phys(start); > - end = sanitize_phys(end); /* * [start, end] is an exclusive address range, but * sanitize_phys() expects an inclusive end address */ > + end = sanitize_phys(end - 1) + 1; > if (start >= end) { > WARN(1, "%s failed: [mem %#010Lx-%#010Lx], req %s\n", __func__, > start, end - 1, cattr_name(req_type)); Thanks, tglx
Thomas Gleixner <tglx@linutronix.de> writes: > Jeff, > > On Wed, Jul 21 2021 at 15:48, Jeff Moyer wrote: > > Please write function names with brackets, i.e. sanitize_phys(). OK, will do. >> memtype_reserve takes an address range of the form [start, end). It > > [start, end] Start is inclusive, end is exclusive, so start <= x < end. I used the notation found here: https://en.wikipedia.org/wiki/Interval_(mathematics) If that's too confusing, I can stick to inclusive vs exclusive verbiage. >> then passes the start and end addresses to sanitize_phys, which is meant >> to operate on the inclusive addresses. If end falls at the end of the >> physical address space, sanitize_phys will return 0. This can result in >> drivers failing to load: >> >> [ 10.000087] mpt3sas_cm0: unable to map adapter memory! or resource not found >> [ 10.000334] mpt3sas_cm0: failure at drivers/scsi/mpt3sas/mpt3sas_scsih.c:10597/_scsih_probe()! > > Doesn't this trigger the WARN() right below that offending line? It does. I'll include the warning message in the v2 posting. >> Fix this by passing the inclusive end address to sanitize_phys. >> >> Fixes: 510ee090abc3 ("x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses") >> Signed-off-by: Jeff Moyer <jmoyer@redhat.com> >> -- >> It might be worth adding a comment, here. If there are any suggestions >> on what a sane wording would be, I'm all ears. > > See below. > >> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c >> index 3112ca7786ed..482557905294 100644 >> --- a/arch/x86/mm/pat/memtype.c >> +++ b/arch/x86/mm/pat/memtype.c >> @@ -583,7 +583,7 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type, >> int err = 0; >> >> start = sanitize_phys(start); >> - end = sanitize_phys(end); > > /* > * [start, end] is an exclusive address range, but > * sanitize_phys() expects an inclusive end address > */ That works for me (modulo the interval notation), thanks for the suggestion. Thanks! Jeff
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c index 3112ca7786ed..482557905294 100644 --- a/arch/x86/mm/pat/memtype.c +++ b/arch/x86/mm/pat/memtype.c @@ -583,7 +583,7 @@ int memtype_reserve(u64 start, u64 end, enum page_cache_mode req_type, int err = 0; start = sanitize_phys(start); - end = sanitize_phys(end); + end = sanitize_phys(end - 1) + 1; if (start >= end) { WARN(1, "%s failed: [mem %#010Lx-%#010Lx], req %s\n", __func__, start, end - 1, cattr_name(req_type));
memtype_reserve takes an address range of the form [start, end). It then passes the start and end addresses to sanitize_phys, which is meant to operate on the inclusive addresses. If end falls at the end of the physical address space, sanitize_phys will return 0. This can result in drivers failing to load: [ 10.000087] mpt3sas_cm0: unable to map adapter memory! or resource not found [ 10.000334] mpt3sas_cm0: failure at drivers/scsi/mpt3sas/mpt3sas_scsih.c:10597/_scsih_probe()! Fix this by passing the inclusive end address to sanitize_phys. Fixes: 510ee090abc3 ("x86/mm/pat: Prepare {reserve, free}_memtype() for "decoy" addresses") Signed-off-by: Jeff Moyer <jmoyer@redhat.com> -- It might be worth adding a comment, here. If there are any suggestions on what a sane wording would be, I'm all ears.