* [PATCH 0/2] mm, slab: consolidate KMALLOC_MAX_SIZE @ 2016-12-15 16:47 Michal Hocko 2016-12-15 16:47 ` [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX Michal Hocko 2016-12-15 16:47 ` [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER Michal Hocko 0 siblings, 2 replies; 13+ messages in thread From: Michal Hocko @ 2016-12-15 16:47 UTC (permalink / raw) To: linux-mm Cc: Cristopher Lameter, Andrew Morton, Alexei Starovoitov, Andrey Konovalov, Michal Hocko Hi, Andrey has revealed a discrepancy between KMALLOC_MAX_SIZE and the maximum supported page allocator size [1]. The underlying problem should be fixed in the ep_write_iter code of course, but I do not feel qualified to do that. The discrepancy which it reveals (see patch 2) is worth fixing anyway, though. While I was looking into the code, I've noticed that the only code which uses KMALLOC_SHIFT_MAX outside of the slab code is bpf so I've updated it to use KMALLOC_MAX_SIZE instead. There shouldn't be any real reason to use KMALLOC_SHIFT_MAX which is a slab internal constant same as KMALLOC_SHIFT_{LOW,HIGH} [1] http://lkml.kernel.org/r/CAAeHK+ztusS68DejO8AH3nn-EfiYQpD5FmBwmqKG8BWvoqPNqQ@mail.gmail.com -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-15 16:47 [PATCH 0/2] mm, slab: consolidate KMALLOC_MAX_SIZE Michal Hocko @ 2016-12-15 16:47 ` Michal Hocko 2016-12-15 19:06 ` Christoph Lameter 2016-12-16 18:02 ` Alexei Starovoitov 2016-12-15 16:47 ` [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER Michal Hocko 1 sibling, 2 replies; 13+ messages in thread From: Michal Hocko @ 2016-12-15 16:47 UTC (permalink / raw) To: linux-mm Cc: Cristopher Lameter, Andrew Morton, Michal Hocko, Alexei Starovoitov From: Michal Hocko <mhocko@suse.com> 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer overflow") has added checks for the maximum allocateable size. It (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect it is not very clean because we already have KMALLOC_MAX_SIZE for this very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. Cc: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Michal Hocko <mhocko@suse.com> --- kernel/bpf/arraymap.c | 2 +- kernel/bpf/hashtab.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index a2ac051c342f..229a5d5df977 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -56,7 +56,7 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr) attr->value_size == 0 || attr->map_flags) return ERR_PTR(-EINVAL); - if (attr->value_size >= 1 << (KMALLOC_SHIFT_MAX - 1)) + if (attr->value_size > KMALLOC_MAX_SIZE) /* if value_size is bigger, the user space won't be able to * access the elements. */ diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index ad1bc67aff1b..c5ec7dc71c84 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -181,7 +181,7 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr) */ goto free_htab; - if (htab->map.value_size >= (1 << (KMALLOC_SHIFT_MAX - 1)) - + if (htab->map.value_size >= KMALLOC_MAX_SIZE - MAX_BPF_STACK - sizeof(struct htab_elem)) /* if value_size is bigger, the user space won't be able to * access the elements via bpf syscall. This check also makes -- 2.10.2 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-15 16:47 ` [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX Michal Hocko @ 2016-12-15 19:06 ` Christoph Lameter 2016-12-16 18:02 ` Alexei Starovoitov 1 sibling, 0 replies; 13+ messages in thread From: Christoph Lameter @ 2016-12-15 19:06 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-mm, Andrew Morton, Michal Hocko, Alexei Starovoitov On Thu, 15 Dec 2016, Michal Hocko wrote: > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > overflow") has added checks for the maximum allocateable size. It > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > it is not very clean because we already have KMALLOC_MAX_SIZE for this > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. Acked-by: Christoph Lameter <cl@linux.com> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-15 16:47 ` [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX Michal Hocko 2016-12-15 19:06 ` Christoph Lameter @ 2016-12-16 18:02 ` Alexei Starovoitov 2016-12-16 22:02 ` Michal Hocko 1 sibling, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2016-12-16 18:02 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, Cristopher Lameter, Andrew Morton, Michal Hocko, Alexei Starovoitov, netdev, Daniel Borkmann On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > From: Michal Hocko <mhocko@suse.com> > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > overflow") has added checks for the maximum allocateable size. It > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > it is not very clean because we already have KMALLOC_MAX_SIZE for this > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > Cc: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Michal Hocko <mhocko@suse.com> Nack until the patches 1 and 2 are reversed. The bug that patch 2 fixes was the reason we used KMALLOC_SHIFT_MAX - 1 here instead of KMALLOC_MAX_SIZE, so you have to fix the kmalloc vs __alloc_pages_slowpath discrepancy first. > --- > kernel/bpf/arraymap.c | 2 +- > kernel/bpf/hashtab.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > index a2ac051c342f..229a5d5df977 100644 > --- a/kernel/bpf/arraymap.c > +++ b/kernel/bpf/arraymap.c > @@ -56,7 +56,7 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr) > attr->value_size == 0 || attr->map_flags) > return ERR_PTR(-EINVAL); > > - if (attr->value_size >= 1 << (KMALLOC_SHIFT_MAX - 1)) > + if (attr->value_size > KMALLOC_MAX_SIZE) > /* if value_size is bigger, the user space won't be able to > * access the elements. > */ > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c > index ad1bc67aff1b..c5ec7dc71c84 100644 > --- a/kernel/bpf/hashtab.c > +++ b/kernel/bpf/hashtab.c > @@ -181,7 +181,7 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr) > */ > goto free_htab; > > - if (htab->map.value_size >= (1 << (KMALLOC_SHIFT_MAX - 1)) - > + if (htab->map.value_size >= KMALLOC_MAX_SIZE - > MAX_BPF_STACK - sizeof(struct htab_elem)) > /* if value_size is bigger, the user space won't be able to > * access the elements via bpf syscall. This check also makes > -- > 2.10.2 > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-16 18:02 ` Alexei Starovoitov @ 2016-12-16 22:02 ` Michal Hocko 2016-12-16 23:23 ` Alexei Starovoitov 0 siblings, 1 reply; 13+ messages in thread From: Michal Hocko @ 2016-12-16 22:02 UTC (permalink / raw) To: Alexei Starovoitov Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > From: Michal Hocko <mhocko@suse.com> > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > overflow") has added checks for the maximum allocateable size. It > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > Nack until the patches 1 and 2 are reversed. I do not insist on ordering. The thing is that it shouldn't matter all that much. Or are you worried about bisectability? -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-16 22:02 ` Michal Hocko @ 2016-12-16 23:23 ` Alexei Starovoitov 0 siblings, 0 replies; 13+ messages in thread From: Alexei Starovoitov @ 2016-12-16 23:23 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > > From: Michal Hocko <mhocko@suse.com> > > > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > > overflow") has added checks for the maximum allocateable size. It > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > > Nack until the patches 1 and 2 are reversed. > > I do not insist on ordering. The thing is that it shouldn't matter all > that much. Or are you worried about bisectability? This patch 1 strongly depends on patch 2 ! Therefore order matters. The patch 1 by itself is broken. The commit log is saying '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. So please change the order and fix the commit log to say that KMALLOC_MAX_SIZE is actually valid limit now. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX @ 2016-12-16 23:23 ` Alexei Starovoitov 0 siblings, 0 replies; 13+ messages in thread From: Alexei Starovoitov @ 2016-12-16 23:23 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > > From: Michal Hocko <mhocko@suse.com> > > > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > > overflow") has added checks for the maximum allocateable size. It > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > > Nack until the patches 1 and 2 are reversed. > > I do not insist on ordering. The thing is that it shouldn't matter all > that much. Or are you worried about bisectability? This patch 1 strongly depends on patch 2 ! Therefore order matters. The patch 1 by itself is broken. The commit log is saying '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. So please change the order and fix the commit log to say that KMALLOC_MAX_SIZE is actually valid limit now. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-16 23:23 ` Alexei Starovoitov (?) @ 2016-12-16 23:39 ` Michal Hocko 2016-12-17 0:28 ` Alexei Starovoitov -1 siblings, 1 reply; 13+ messages in thread From: Michal Hocko @ 2016-12-16 23:39 UTC (permalink / raw) To: Alexei Starovoitov Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Fri 16-12-16 15:23:42, Alexei Starovoitov wrote: > On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: > > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > > > From: Michal Hocko <mhocko@suse.com> > > > > > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > > > overflow") has added checks for the maximum allocateable size. It > > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > > > > Nack until the patches 1 and 2 are reversed. > > > > I do not insist on ordering. The thing is that it shouldn't matter all > > that much. Or are you worried about bisectability? > > This patch 1 strongly depends on patch 2 ! > Therefore order matters. > The patch 1 by itself is broken. > The commit log is saying > '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' > that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. > So please change the order Yes, I agree that using KMALLOC_MAX_SIZE could lead to a warning with the current ordering. Why that matters all that much is less clear to me. The allocation would simply fail and you would return ENOMEM rather than E2BIG. Does this really matter? Anyway, as I've said, I do not really insist on the current ordering and the will ask Andrew to reorder them. I am just really wondering about such a strong pushback about something that barely matters. Or maybe I am just missing your point and checking KMALLOC_MAX_SIZE without an update would lead to a wrong behavior, user space breakage, crash or anything similar. > and fix the commit log to say that KMALLOC_MAX_SIZE > is actually valid limit now. KMALLOC_MAX_SIZE has always been the right limit. It's value has been incorrect but that is to be fixed now. Using KMALLOC_SHIFT_MAX is simply abusing an internal constant. So I am not sure what should be fixed in the changelog. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-16 23:39 ` Michal Hocko @ 2016-12-17 0:28 ` Alexei Starovoitov 2016-12-17 8:27 ` Michal Hocko 0 siblings, 1 reply; 13+ messages in thread From: Alexei Starovoitov @ 2016-12-17 0:28 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Sat, Dec 17, 2016 at 12:39:17AM +0100, Michal Hocko wrote: > On Fri 16-12-16 15:23:42, Alexei Starovoitov wrote: > > On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: > > > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > > > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > > > > From: Michal Hocko <mhocko@suse.com> > > > > > > > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > > > > overflow") has added checks for the maximum allocateable size. It > > > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > > > > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > > > > > > Nack until the patches 1 and 2 are reversed. > > > > > > I do not insist on ordering. The thing is that it shouldn't matter all > > > that much. Or are you worried about bisectability? > > > > This patch 1 strongly depends on patch 2 ! > > Therefore order matters. > > The patch 1 by itself is broken. > > The commit log is saying > > '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' > > that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. > > So please change the order > > Yes, I agree that using KMALLOC_MAX_SIZE could lead to a warning with > the current ordering. Why that matters all that much is less clear to > me. The allocation would simply fail and you would return ENOMEM rather > than E2BIG. Does this really matter? > > Anyway, as I've said, I do not really insist on the current ordering and > the will ask Andrew to reorder them. I am just really wondering about > such a strong pushback about something that barely matters. Or maybe I > am just missing your point and checking KMALLOC_MAX_SIZE without an > update would lead to a wrong behavior, user space breakage, crash or > anything similar. if admin set ulimit for locked memory high enough for the particular user, that non-root user will be able to trigger warn_on_once in __alloc_pages_slowpath which is not acceptable. Also see the comment in hashtab.c if (htab->map.value_size >= (1 << (KMALLOC_SHIFT_MAX - 1)) - MAX_BPF_STACK - sizeof(struct htab_elem)) /* if value_size is bigger, the user space won't be able to * access the elements via bpf syscall. This check also makes * sure that the elem_size doesn't overflow and it's * kmalloc-able later in htab_map_update_elem() */ goto free_htab; > > and fix the commit log to say that KMALLOC_MAX_SIZE > > is actually valid limit now. > > KMALLOC_MAX_SIZE has always been the right limit. It's value has been > incorrect but that is to be fixed now. Using KMALLOC_SHIFT_MAX is simply > abusing an internal constant. So I am not sure what should be fixed in > the changelog. that's exactly my problem with this patch and the commit log. You think it's abusing KMALLOC_SHIFT_MAX whereas it's doing so for reasons stated above. That piece of code cannot use KMALLOC_MAX_SIZE until it's fixed. So commit log should say something like: "now since KMALLOC_MAX_SIZE is fixed and size < KMALLOC_MAX_SIZE condition guarantees warn free allocation in kmalloc(value_size, GFP_USER | __GFP_NOWARN); we can safely use KMALLOC_MAX_SIZE instead of KMALLOC_SHIFT_MAX" -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-17 0:28 ` Alexei Starovoitov @ 2016-12-17 8:27 ` Michal Hocko 0 siblings, 0 replies; 13+ messages in thread From: Michal Hocko @ 2016-12-17 8:27 UTC (permalink / raw) To: Alexei Starovoitov Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev, Daniel Borkmann On Fri 16-12-16 16:28:21, Alexei Starovoitov wrote: > On Sat, Dec 17, 2016 at 12:39:17AM +0100, Michal Hocko wrote: > > On Fri 16-12-16 15:23:42, Alexei Starovoitov wrote: > > > On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: > > > > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: > > > > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: > > > > > > From: Michal Hocko <mhocko@suse.com> > > > > > > > > > > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer > > > > > > overflow") has added checks for the maximum allocateable size. It > > > > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect > > > > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this > > > > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. > > > > > > > > > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > > > > > > > > Nack until the patches 1 and 2 are reversed. > > > > > > > > I do not insist on ordering. The thing is that it shouldn't matter all > > > > that much. Or are you worried about bisectability? > > > > > > This patch 1 strongly depends on patch 2 ! > > > Therefore order matters. > > > The patch 1 by itself is broken. > > > The commit log is saying > > > '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' > > > that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. > > > So please change the order > > > > Yes, I agree that using KMALLOC_MAX_SIZE could lead to a warning with > > the current ordering. Why that matters all that much is less clear to > > me. The allocation would simply fail and you would return ENOMEM rather > > than E2BIG. Does this really matter? > > > > Anyway, as I've said, I do not really insist on the current ordering and > > the will ask Andrew to reorder them. I am just really wondering about > > such a strong pushback about something that barely matters. Or maybe I > > am just missing your point and checking KMALLOC_MAX_SIZE without an > > update would lead to a wrong behavior, user space breakage, crash or > > anything similar. > > if admin set ulimit for locked memory high enough for the particular user, > that non-root user will be able to trigger warn_on_once in __alloc_pages_slowpath > which is not acceptable. But why is the warning such a big deal? Also note that such a setup would be inherently dangerous. Even the default ulimit for the locked memory allows to allocat 64k which means that an untrusted user will be able to request PAGE_ALLOC_COSTLY_ORDER and potentially deplete those larger blocks to the extend it hits the OOM killer with the current gfp flags. I think what you really want is a GFP_NORETRY for size > PAGE_SIZE and fallback to the vmalloc for failure. But that is a separate topic. > Also see the comment in hashtab.c > if (htab->map.value_size >= (1 << (KMALLOC_SHIFT_MAX - 1)) - > MAX_BPF_STACK - sizeof(struct htab_elem)) > /* if value_size is bigger, the user space won't be able to > * access the elements via bpf syscall. This check also makes > * sure that the elem_size doesn't overflow and it's > * kmalloc-able later in htab_map_update_elem() > */ > goto free_htab; I have seen this comment before, but honestly, I do not understand it (well apart from the overflow part). htab_map_update_elem has to be able to handle the allocation failure in any case. Note that any allocation larger than 64kB is likely to fail anyway. > > > > and fix the commit log to say that KMALLOC_MAX_SIZE > > > is actually valid limit now. > > > > KMALLOC_MAX_SIZE has always been the right limit. It's value has been > > incorrect but that is to be fixed now. Using KMALLOC_SHIFT_MAX is simply > > abusing an internal constant. So I am not sure what should be fixed in > > the changelog. > > that's exactly my problem with this patch and the commit log. > You think it's abusing KMALLOC_SHIFT_MAX whereas it's doing so > for reasons stated above. > That piece of code cannot use KMALLOC_MAX_SIZE until it's fixed. > So commit log should say something like: > "now since KMALLOC_MAX_SIZE is fixed and size < KMALLOC_MAX_SIZE condition > guarantees warn free allocation in kmalloc(value_size, GFP_USER | __GFP_NOWARN); > we can safely use KMALLOC_MAX_SIZE instead of KMALLOC_SHIFT_MAX" OK, fair enough, I will update the changelog -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX 2016-12-16 23:23 ` Alexei Starovoitov (?) (?) @ 2016-12-16 23:40 ` Daniel Borkmann -1 siblings, 0 replies; 13+ messages in thread From: Daniel Borkmann @ 2016-12-16 23:40 UTC (permalink / raw) To: Alexei Starovoitov, Michal Hocko Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov, netdev On 12/17/2016 12:23 AM, Alexei Starovoitov wrote: > On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote: >> On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote: >>> On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote: >>>> From: Michal Hocko <mhocko@suse.com> >>>> >>>> 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer >>>> overflow") has added checks for the maximum allocateable size. It >>>> (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect >>>> it is not very clean because we already have KMALLOC_MAX_SIZE for this >>>> very reason so let's change both checks to use KMALLOC_MAX_SIZE instead. >>>> >>>> Cc: Alexei Starovoitov <ast@kernel.org> >>>> Signed-off-by: Michal Hocko <mhocko@suse.com> >>> >>> Nack until the patches 1 and 2 are reversed. >> >> I do not insist on ordering. The thing is that it shouldn't matter all >> that much. Or are you worried about bisectability? > > This patch 1 strongly depends on patch 2 ! > Therefore order matters. > The patch 1 by itself is broken. > The commit log is saying > '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead' > that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed. > So please change the order and fix the commit log to say that KMALLOC_MAX_SIZE > is actually valid limit now. Michal, please also Cc netdev on your v2. Looks like the set originally didn't Cc it (at least I didn't see 2/2). Thanks. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER 2016-12-15 16:47 [PATCH 0/2] mm, slab: consolidate KMALLOC_MAX_SIZE Michal Hocko 2016-12-15 16:47 ` [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX Michal Hocko @ 2016-12-15 16:47 ` Michal Hocko 2016-12-15 19:11 ` Christoph Lameter 1 sibling, 1 reply; 13+ messages in thread From: Michal Hocko @ 2016-12-15 16:47 UTC (permalink / raw) To: linux-mm; +Cc: Cristopher Lameter, Andrew Morton, Michal Hocko From: Michal Hocko <mhocko@suse.com> Andrey Konovalov has reported the following warning triggered by the syzkaller fuzzer. WARNING: CPU: 1 PID: 9935 at mm/page_alloc.c:3511 __alloc_pages_nodemask+0x159c/0x1e20 Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 9935 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #34 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 ffff88006949f2c8 ffffffff81f96b8a ffffffff00000200 1ffff1000d293dec ffffed000d293de4 0000000000000a06 0000000041b58ab3 ffffffff8598b510 ffffffff81f968f8 0000000041b58ab3 ffffffff85942a58 ffffffff81432860 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [<ffffffff81f96b8a>] dump_stack+0x292/0x398 lib/dump_stack.c:51 [<ffffffff8168c88e>] panic+0x1cb/0x3a9 kernel/panic.c:179 [<ffffffff812b80b4>] __warn+0x1c4/0x1e0 kernel/panic.c:542 [<ffffffff812b831c>] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 [< inline >] __alloc_pages_slowpath mm/page_alloc.c:3511 [<ffffffff816c08ac>] __alloc_pages_nodemask+0x159c/0x1e20 mm/page_alloc.c:3781 [<ffffffff817cde17>] alloc_pages_current+0x1c7/0x6b0 mm/mempolicy.c:2072 [< inline >] alloc_pages include/linux/gfp.h:469 [<ffffffff8172fd8f>] kmalloc_order+0x1f/0x70 mm/slab_common.c:1015 [<ffffffff8172fdff>] kmalloc_order_trace+0x1f/0x160 mm/slab_common.c:1026 [< inline >] kmalloc_large include/linux/slab.h:422 [<ffffffff817e01f0>] __kmalloc+0x210/0x2d0 mm/slub.c:3723 [< inline >] kmalloc include/linux/slab.h:495 [<ffffffff832262a7>] ep_write_iter+0x167/0xb50 drivers/usb/gadget/legacy/inode.c:664 [< inline >] new_sync_write fs/read_write.c:499 [<ffffffff817fdcd3>] __vfs_write+0x483/0x760 fs/read_write.c:512 [<ffffffff817ff720>] vfs_write+0x170/0x4e0 fs/read_write.c:560 [< inline >] SYSC_write fs/read_write.c:607 [<ffffffff81803b2b>] SyS_write+0xfb/0x230 fs/read_write.c:599 [<ffffffff84f47ec1>] entry_SYSCALL_64_fastpath+0x1f/0xc2 The issue is caused by a lack of size check for the request size in ep_write_iter which should be fixed. It, however, points to another problem, that SLUB defines KMALLOC_MAX_SIZE too large because the its KMALLOC_SHIFT_MAX is (MAX_ORDER + PAGE_SHIFT) which means that the resulting page allocator request might be MAX_ORDER which is too large (see __alloc_pages_slowpath). The same applies to the SLOB allocator which allows even larger sizes. Make sure that they are capped properly and never request more than MAX_ORDER order. Reported-by: Andrey Konovalov <andreyknvl@google.com> Signed-off-by: Michal Hocko <mhocko@suse.com> --- include/linux/slab.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index 084b12bad198..4c5363566815 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -226,7 +226,7 @@ static inline const char *__check_heap_object(const void *ptr, * (PAGE_SIZE*2). Larger requests are passed to the page allocator. */ #define KMALLOC_SHIFT_HIGH (PAGE_SHIFT + 1) -#define KMALLOC_SHIFT_MAX (MAX_ORDER + PAGE_SHIFT) +#define KMALLOC_SHIFT_MAX (MAX_ORDER + PAGE_SHIFT - 1) #ifndef KMALLOC_SHIFT_LOW #define KMALLOC_SHIFT_LOW 3 #endif @@ -239,7 +239,7 @@ static inline const char *__check_heap_object(const void *ptr, * be allocated from the same page. */ #define KMALLOC_SHIFT_HIGH PAGE_SHIFT -#define KMALLOC_SHIFT_MAX 30 +#define KMALLOC_SHIFT_MAX (MAX_ORDER + PAGE_SHIFT - 1) #ifndef KMALLOC_SHIFT_LOW #define KMALLOC_SHIFT_LOW 3 #endif -- 2.10.2 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER 2016-12-15 16:47 ` [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER Michal Hocko @ 2016-12-15 19:11 ` Christoph Lameter 0 siblings, 0 replies; 13+ messages in thread From: Christoph Lameter @ 2016-12-15 19:11 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-mm, Andrew Morton, Michal Hocko On Thu, 15 Dec 2016, Michal Hocko wrote: > (see __alloc_pages_slowpath). The same applies to the SLOB allocator > which allows even larger sizes. Make sure that they are capped properly > and never request more than MAX_ORDER order. Acked-by: Christoph Lameter <cl@linux.com> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-12-17 8:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-15 16:47 [PATCH 0/2] mm, slab: consolidate KMALLOC_MAX_SIZE Michal Hocko 2016-12-15 16:47 ` [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX Michal Hocko 2016-12-15 19:06 ` Christoph Lameter 2016-12-16 18:02 ` Alexei Starovoitov 2016-12-16 22:02 ` Michal Hocko 2016-12-16 23:23 ` Alexei Starovoitov 2016-12-16 23:23 ` Alexei Starovoitov 2016-12-16 23:39 ` Michal Hocko 2016-12-17 0:28 ` Alexei Starovoitov 2016-12-17 8:27 ` Michal Hocko 2016-12-16 23:40 ` Daniel Borkmann 2016-12-15 16:47 ` [PATCH 2/2] mm, slab: make sure that KMALLOC_MAX_SIZE will fit into MAX_ORDER Michal Hocko 2016-12-15 19:11 ` Christoph Lameter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.