| Message ID | 20191215154432.22399-1-pakki001@umn.edu |
|---|---|
| State | Rejected |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Replace BUG_ON when fp_old is NULL | expand |
On 12/15/19 7:44 AM, Aditya Pakki wrote: > If fp_old is NULL in bpf_prog_realloc, the program does an assertion > and crashes. However, we can continue execution by returning NULL to > the upper callers. The patch fixes this issue. Could you share how to reproduce the assertion and crash? I would like to understand the problem first before making changes in the code. Thanks! > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > kernel/bpf/core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 49e32acad7d8..4b46654fb26b 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -222,7 +222,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > u32 pages, delta; > int ret; > > - BUG_ON(fp_old == NULL); > + if (!fp_old) > + return NULL; > > size = round_up(size, PAGE_SIZE); > pages = size / PAGE_SIZE; >
On Sun, Dec 15, 2019 at 09:44:32AM -0600, Aditya Pakki wrote: > If fp_old is NULL in bpf_prog_realloc, the program does an assertion > and crashes. However, we can continue execution by returning NULL to > the upper callers. The patch fixes this issue. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > kernel/bpf/core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > index 49e32acad7d8..4b46654fb26b 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -222,7 +222,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, > u32 pages, delta; > int ret; > > - BUG_ON(fp_old == NULL); > + if (!fp_old) > + return NULL; This change makes no sense to me. fp_old should be valid. That's the point of BUG_ON. It can happen only during development. Can remove BUG_ON just as well and let kernel page fault.
On 12/15/19 11:08 PM, Yonghong Song wrote: > On 12/15/19 7:44 AM, Aditya Pakki wrote: >> If fp_old is NULL in bpf_prog_realloc, the program does an assertion >> and crashes. However, we can continue execution by returning NULL to >> the upper callers. The patch fixes this issue. > > Could you share how to reproduce the assertion and crash? I would > like to understand the problem first before making changes in the code. > Thanks! Fully agree, Aditya, please elaborate if you have seen a crash!
On 12/16/19 5:17 AM, Daniel Borkmann wrote: > On 12/15/19 11:08 PM, Yonghong Song wrote: >> On 12/15/19 7:44 AM, Aditya Pakki wrote: >>> If fp_old is NULL in bpf_prog_realloc, the program does an assertion >>> and crashes. However, we can continue execution by returning NULL to >>> the upper callers. The patch fixes this issue. >> >> Could you share how to reproduce the assertion and crash? I would >> like to understand the problem first before making changes in the code. >> Thanks! > > Fully agree, Aditya, please elaborate if you have seen a crash! Thanks for your responses Alexei and Daniel. We identified this issue via static analysis and have not seen a crash. However, by looking at the callers of bpf_prog_realloc, I do agree that fp_old is never NULL. Would you recommend removing the BUG_ON assertion altogether ?
On 12/19/19 6:39 PM, Aditya Pakki wrote: > On 12/16/19 5:17 AM, Daniel Borkmann wrote: >> On 12/15/19 11:08 PM, Yonghong Song wrote: >>> On 12/15/19 7:44 AM, Aditya Pakki wrote: >>>> If fp_old is NULL in bpf_prog_realloc, the program does an assertion >>>> and crashes. However, we can continue execution by returning NULL to >>>> the upper callers. The patch fixes this issue. >>> >>> Could you share how to reproduce the assertion and crash? I would >>> like to understand the problem first before making changes in the code. >>> Thanks! >> >> Fully agree, Aditya, please elaborate if you have seen a crash! > > Thanks for your responses Alexei and Daniel. We identified this issue via static analysis > and have not seen a crash. However, by looking at the callers of bpf_prog_realloc, I do > agree that fp_old is never NULL. > > Would you recommend removing the BUG_ON assertion altogether ? If it would ever happen, we'd already crash in fp_old->pages there, so yes, lets remove the unneeded BUG_ON(). Thanks, Daniel
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 49e32acad7d8..4b46654fb26b 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -222,7 +222,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size, u32 pages, delta; int ret; - BUG_ON(fp_old == NULL); + if (!fp_old) + return NULL; size = round_up(size, PAGE_SIZE); pages = size / PAGE_SIZE;
If fp_old is NULL in bpf_prog_realloc, the program does an assertion and crashes. However, we can continue execution by returning NULL to the upper callers. The patch fixes this issue. Signed-off-by: Aditya Pakki <pakki001@umn.edu> --- kernel/bpf/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)