| Message ID | 20120807085554.GF29814@suse.de |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/07/2012 04:55 AM, Mel Gorman wrote: > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > for the following bug triggered by a xen network driver > > [ 1.908592] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > [ 1.908643] IP: [<ffffffffa0037750>] xennet_poll+0x980/0xec0 [xen_netfront] > [ 1.908703] PGD ea1df067 PUD e8ada067 PMD 0 > [ 1.908774] Oops: 0000 [#1] SMP > [ 1.908797] Modules linked in: fbcon tileblit font radeon bitblit softcursor ttm drm_kms_helper crc32c_intel xen_blkfront xen_netfront xen_fbfront fb_sys_fops sysimgblt sysfillrect syscopyarea +xen_kbdfront xenfs xen_privcmd > [ 1.908938] CPU 0 > [ 1.908950] Pid: 2165, comm: ip Not tainted 3.5.0upstream-08854-g444fa66 #1 > [ 1.908983] RIP: e030:[<ffffffffa0037750>] [<ffffffffa0037750>] xennet_poll+0x980/0xec0 [xen_netfront] > [ 1.909029] RSP: e02b:ffff8800ffc03db8 EFLAGS: 00010282 > [ 1.909055] RAX: ffff8800ea010140 RBX: ffff8800f00e86c0 RCX: 000000000000009a > [ 1.909055] RDX: 0000000000000040 RSI: 000000000000005a RDI: ffff8800fa7dee80 > [ 1.909055] RBP: ffff8800ffc03ee8 R08: ffff8800f00e86d8 R09: ffff8800ea010000 > [ 1.909055] R10: dead000000200200 R11: dead000000100100 R12: ffff8800fa7dee80 > [ 1.909055] R13: 000000000000005a R14: ffff8800fa7dee80 R15: 0000000000000200 > [ 1.909055] FS: 00007fbafc188700(0000) GS:ffff8800ffc00000(0000) knlGS:0000000000000000 > [ 1.909055] CS: e033 DS: 0000 ES: 0000 CR0: 000000008005003b > [ 1.909055] CR2: 0000000000000010 CR3: 00000000ea108000 CR4: 0000000000002660 > [ 1.909055] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 1.909055] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > [ 1.909055] Process ip (pid: 2165, threadinfo ffff8800ea0f2000, task ffff8800fa783040) > [ 1.909055] Stack: > [ 1.909055] ffff8800e27e5040 ffff8800ffc03e88 ffff8800ffc03e68 ffff8800ffc03e48 > [ 1.909055] 7fffffffffffffff ffff8800ffc03e00 ffff8800e27e5040 ffff8800f00e86d8 > [ 1.909055] ffff8800ffc03eb0 00000040ffffffff ffff8800f00e8000 00000000ffc03e30 > [ 1.909055] Call Trace: > [ 1.909055] <IRQ> > [ 1.909055] [<ffffffff81066028>] ? pvclock_clocksource_read+0x58/0xd0 > [ 1.909055] [<ffffffff81486352>] net_rx_action+0x112/0x240 > [ 1.909055] [<ffffffff8107f319>] __do_softirq+0xb9/0x190 > [ 1.909055] [<ffffffff815d8d7c>] call_softirq+0x1c/0x30 > > The problem is that the xenfront driver is passing a NULL page to > __skb_fill_page_desc() which was unexpected. This patch checks that > there is a page before dereferencing. > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Signed-off-by: Mel Gorman <mgorman@suse.de> Acked-by: Rik van Riel <riel@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mel Gorman <mgorman@suse.de> Date: Tue, 7 Aug 2012 09:55:55 +0100 > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > for the following bug triggered by a xen network driver ... > The problem is that the xenfront driver is passing a NULL page to > __skb_fill_page_desc() which was unexpected. This patch checks that > there is a page before dereferencing. > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Signed-off-by: Mel Gorman <mgorman@suse.de> That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. It's the only driver passing NULL here. That whole song and dance figuring out what to do with the head fragment page, depending upon whether the length is greater than the RX_COPY_THRESHOLD, is completely unnecessary. Just use something like a call to __pskb_pull_tail(skb, len) and all that other crap around that area can simply be deleted. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > From: Mel Gorman <mgorman@suse.de> > Date: Tue, 7 Aug 2012 09:55:55 +0100 > > > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > > for the following bug triggered by a xen network driver > ... > > The problem is that the xenfront driver is passing a NULL page to > > __skb_fill_page_desc() which was unexpected. This patch checks that > > there is a page before dereferencing. > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Signed-off-by: Mel Gorman <mgorman@suse.de> > > That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > It's the only driver passing NULL here. > > That whole song and dance figuring out what to do with the head > fragment page, depending upon whether the length is greater than the > RX_COPY_THRESHOLD, is completely unnecessary. > > Just use something like a call to __pskb_pull_tail(skb, len) and all > that other crap around that area can simply be deleted. I looked at this for a while but I did not see how __pskb_pull_tail() could be used sensibly but I'm simily not familiar with writing network device drivers or Xen. This messing with RX_COPY_THRESHOLD seems to be related to how the frontend and backend communicate (maybe some fixed limitation of the xenbus). The existing code looks like it is trying to take the fragments received and pass them straight to the backend without copying by passing the fragments to the backend without copying. I worry that if I try converting this to __pskb_pull_tail() that it would either hit the limitation of xenbus or introduce copying where it is not wanted. I'm going to have to punt this to Jeremy and the other Xen folk as I'm not sure what the original intention was and I don't have a Xen setup anywhere to test any patch. Jeremy, xen folk?
Resending to correct Jeremy's address. On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > From: Mel Gorman <mgorman@suse.de> > Date: Tue, 7 Aug 2012 09:55:55 +0100 > > > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > > for the following bug triggered by a xen network driver > ... > > The problem is that the xenfront driver is passing a NULL page to > > __skb_fill_page_desc() which was unexpected. This patch checks that > > there is a page before dereferencing. > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Signed-off-by: Mel Gorman <mgorman@suse.de> > > That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > It's the only driver passing NULL here. > > That whole song and dance figuring out what to do with the head > fragment page, depending upon whether the length is greater than the > RX_COPY_THRESHOLD, is completely unnecessary. > > Just use something like a call to __pskb_pull_tail(skb, len) and all > that other crap around that area can simply be deleted. I looked at this for a while but I did not see how __pskb_pull_tail() could be used sensibly but I'm simily not familiar with writing network device drivers or Xen. This messing with RX_COPY_THRESHOLD seems to be related to how the frontend and backend communicate (maybe some fixed limitation of the xenbus). The existing code looks like it is trying to take the fragments received and pass them straight to the backend without copying by passing the fragments to the backend without copying. I worry that if I try converting this to __pskb_pull_tail() that it would either hit the limitation of xenbus or introduce copying where it is not wanted. I'm going to have to punt this to Jeremy and the other Xen folk as I'm not sure what the original intention was and I don't have a Xen setup anywhere to test any patch. Jeremy, xen folk?
On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > From: Mel Gorman <mgorman@suse.de> > Date: Tue, 7 Aug 2012 09:55:55 +0100 > > > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > > for the following bug triggered by a xen network driver > ... > > The problem is that the xenfront driver is passing a NULL page to > > __skb_fill_page_desc() which was unexpected. This patch checks that > > there is a page before dereferencing. > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Signed-off-by: Mel Gorman <mgorman@suse.de> > > That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > It's the only driver passing NULL here. It looks to be passing a valid page pointer (at least by looking at the code) so I am not sure how it got turned in a NULL. But let me double-check by instrumenting the driver.. > > That whole song and dance figuring out what to do with the head > fragment page, depending upon whether the length is greater than the > RX_COPY_THRESHOLD, is completely unnecessary. > > Just use something like a call to __pskb_pull_tail(skb, len) and all > that other crap around that area can simply be deleted. It looks like an overkill - it does a lot more than just allocate an SKB and a page. Deleting of extra code would be nice - however I am not going to be able to do that for the next two weeks sadly - as my plate if full of debugging some other stuff. Lets see if Ian has some time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/13/2012 03:47 AM, Mel Gorman wrote: > Resending to correct Jeremy's address. > > On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: >> From: Mel Gorman <mgorman@suse.de> >> Date: Tue, 7 Aug 2012 09:55:55 +0100 >> >>> Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible >>> for the following bug triggered by a xen network driver >> ... >>> The problem is that the xenfront driver is passing a NULL page to >>> __skb_fill_page_desc() which was unexpected. This patch checks that >>> there is a page before dereferencing. >>> >>> Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Signed-off-by: Mel Gorman <mgorman@suse.de> >> That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. >> It's the only driver passing NULL here. >> >> That whole song and dance figuring out what to do with the head >> fragment page, depending upon whether the length is greater than the >> RX_COPY_THRESHOLD, is completely unnecessary. >> >> Just use something like a call to __pskb_pull_tail(skb, len) and all >> that other crap around that area can simply be deleted. > I looked at this for a while but I did not see how __pskb_pull_tail() > could be used sensibly but I'm simily not familiar with writing network > device drivers or Xen. > > This messing with RX_COPY_THRESHOLD seems to be related to how the frontend > and backend communicate (maybe some fixed limitation of the xenbus). The > existing code looks like it is trying to take the fragments received and > pass them straight to the backend without copying by passing the fragments > to the backend without copying. I worry that if I try converting this to > __pskb_pull_tail() that it would either hit the limitation of xenbus or > introduce copying where it is not wanted. > > I'm going to have to punt this to Jeremy and the other Xen folk as I'm not > sure what the original intention was and I don't have a Xen setup anywhere > to test any patch. Jeremy, xen folk? It's been a while since I've looked at that stuff, but as I remember, the issue is that since the packet ring memory is shared with another domain which may be untrustworthy, we want to make copies of the headers before making any decisions based on them so that the other domain can't change them after header processing but before they're actually sent. (The packet payload is considered less important, but of course the same issue applies if you're using some kind of content-aware packet filter.) So that's the rationale for always copying RX_COPY_THRESHOLD, even if the packet is larger than that amount. As far as I know, changing this behaviour wouldn't break the ring protocol, but it does introduce a potential security issue. J -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 13, 2012 at 11:41:44AM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > > From: Mel Gorman <mgorman@suse.de> > > Date: Tue, 7 Aug 2012 09:55:55 +0100 > > > > > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > > > for the following bug triggered by a xen network driver > > ... > > > The problem is that the xenfront driver is passing a NULL page to > > > __skb_fill_page_desc() which was unexpected. This patch checks that > > > there is a page before dereferencing. > > > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > Signed-off-by: Mel Gorman <mgorman@suse.de> > > > > That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > > It's the only driver passing NULL here. > > It looks to be passing a valid page pointer (at least by looking > at the code) so I am not sure how it got turned in a NULL. > Are we looking at different code bases? I see this and I was assuming it was the source of the bug. __skb_fill_page_desc(skb, 0, NULL, 0, 0);
On Mon, Aug 13, 2012 at 11:56:48AM -0700, Jeremy Fitzhardinge wrote: > On 08/13/2012 03:47 AM, Mel Gorman wrote: > > Resending to correct Jeremy's address. > > > > On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > >> From: Mel Gorman <mgorman@suse.de> > >> Date: Tue, 7 Aug 2012 09:55:55 +0100 > >> > >>> Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > >>> for the following bug triggered by a xen network driver > >> ... > >>> The problem is that the xenfront driver is passing a NULL page to > >>> __skb_fill_page_desc() which was unexpected. This patch checks that > >>> there is a page before dereferencing. > >>> > >>> Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >>> Signed-off-by: Mel Gorman <mgorman@suse.de> > >> That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > >> It's the only driver passing NULL here. > >> > >> That whole song and dance figuring out what to do with the head > >> fragment page, depending upon whether the length is greater than the > >> RX_COPY_THRESHOLD, is completely unnecessary. > >> > >> Just use something like a call to __pskb_pull_tail(skb, len) and all > >> that other crap around that area can simply be deleted. > > I looked at this for a while but I did not see how __pskb_pull_tail() > > could be used sensibly but I'm simily not familiar with writing network > > device drivers or Xen. > > > > This messing with RX_COPY_THRESHOLD seems to be related to how the frontend > > and backend communicate (maybe some fixed limitation of the xenbus). The > > existing code looks like it is trying to take the fragments received and > > pass them straight to the backend without copying by passing the fragments > > to the backend without copying. I worry that if I try converting this to > > __pskb_pull_tail() that it would either hit the limitation of xenbus or > > introduce copying where it is not wanted. > > > > I'm going to have to punt this to Jeremy and the other Xen folk as I'm not > > sure what the original intention was and I don't have a Xen setup anywhere > > to test any patch. Jeremy, xen folk? > > It's been a while since I've looked at that stuff, but as I remember, > the issue is that since the packet ring memory is shared with another > domain which may be untrustworthy, we want to make copies of the headers > before making any decisions based on them so that the other domain can't > change them after header processing but before they're actually sent. > (The packet payload is considered less important, but of course the same > issue applies if you're using some kind of content-aware packet filter.) > > So that's the rationale for always copying RX_COPY_THRESHOLD, even if > the packet is larger than that amount. As far as I know, changing this > behaviour wouldn't break the ring protocol, but it does introduce a > potential security issue. > David, This leaves us somewhat in a pickle. If I'm reading this right (which I may not be) it means that using __pskb_pull_tail() will work in the ideal case but potentially introduces a subtle issue in the future. This bug could be "fixed" in the driver by partially reverting [01c68026: xen: netfront: convert to SKB paged frag API.]. That could be viewed as sweeping the problem under the carpet but it does contain the problem to the xen-netfront driver. A new helper could be created like __skb_clear_page_desc but that is overkill for one driver and feels as ugly.
On Tue, Aug 14, 2012 at 11:05:22AM +0100, Mel Gorman wrote: > On Mon, Aug 13, 2012 at 11:41:44AM -0400, Konrad Rzeszutek Wilk wrote: > > On Wed, Aug 08, 2012 at 03:50:46PM -0700, David Miller wrote: > > > From: Mel Gorman <mgorman@suse.de> > > > Date: Tue, 7 Aug 2012 09:55:55 +0100 > > > > > > > Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible > > > > for the following bug triggered by a xen network driver > > > ... > > > > The problem is that the xenfront driver is passing a NULL page to > > > > __skb_fill_page_desc() which was unexpected. This patch checks that > > > > there is a page before dereferencing. > > > > > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > > Signed-off-by: Mel Gorman <mgorman@suse.de> > > > > > > That call to __skb_fill_page_desc() in xen-netfront.c looks completely bogus. > > > It's the only driver passing NULL here. > > > > It looks to be passing a valid page pointer (at least by looking > > at the code) so I am not sure how it got turned in a NULL. > > > > Are we looking at different code bases? I see this and I was assuming it > was the source of the bug. > > __skb_fill_page_desc(skb, 0, NULL, 0, 0); Yes! Well, that is embarrassing. I was looking at the first invocation of __skb_fill_page_desc (which is in xennet_alloc_rx_buffers) <sigh> > > -- > Mel Gorman > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 7632c87..8857669 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1256,7 +1256,7 @@ static inline void __skb_fill_page_desc(struct sk_buff *skb, int i, * do not lose pfmemalloc information as the pages would not be * allocated using __GFP_MEMALLOC. */ - if (page->pfmemalloc && !page->mapping) + if (page && page->pfmemalloc && !page->mapping) skb->pfmemalloc = true; frag->page.p = page; frag->page_offset = off;
Commit [c48a11c7: netvm: propagate page->pfmemalloc to skb] is responsible for the following bug triggered by a xen network driver [ 1.908592] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 [ 1.908643] IP: [<ffffffffa0037750>] xennet_poll+0x980/0xec0 [xen_netfront] [ 1.908703] PGD ea1df067 PUD e8ada067 PMD 0 [ 1.908774] Oops: 0000 [#1] SMP [ 1.908797] Modules linked in: fbcon tileblit font radeon bitblit softcursor ttm drm_kms_helper crc32c_intel xen_blkfront xen_netfront xen_fbfront fb_sys_fops sysimgblt sysfillrect syscopyarea +xen_kbdfront xenfs xen_privcmd [ 1.908938] CPU 0 [ 1.908950] Pid: 2165, comm: ip Not tainted 3.5.0upstream-08854-g444fa66 #1 [ 1.908983] RIP: e030:[<ffffffffa0037750>] [<ffffffffa0037750>] xennet_poll+0x980/0xec0 [xen_netfront] [ 1.909029] RSP: e02b:ffff8800ffc03db8 EFLAGS: 00010282 [ 1.909055] RAX: ffff8800ea010140 RBX: ffff8800f00e86c0 RCX: 000000000000009a [ 1.909055] RDX: 0000000000000040 RSI: 000000000000005a RDI: ffff8800fa7dee80 [ 1.909055] RBP: ffff8800ffc03ee8 R08: ffff8800f00e86d8 R09: ffff8800ea010000 [ 1.909055] R10: dead000000200200 R11: dead000000100100 R12: ffff8800fa7dee80 [ 1.909055] R13: 000000000000005a R14: ffff8800fa7dee80 R15: 0000000000000200 [ 1.909055] FS: 00007fbafc188700(0000) GS:ffff8800ffc00000(0000) knlGS:0000000000000000 [ 1.909055] CS: e033 DS: 0000 ES: 0000 CR0: 000000008005003b [ 1.909055] CR2: 0000000000000010 CR3: 00000000ea108000 CR4: 0000000000002660 [ 1.909055] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1.909055] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [ 1.909055] Process ip (pid: 2165, threadinfo ffff8800ea0f2000, task ffff8800fa783040) [ 1.909055] Stack: [ 1.909055] ffff8800e27e5040 ffff8800ffc03e88 ffff8800ffc03e68 ffff8800ffc03e48 [ 1.909055] 7fffffffffffffff ffff8800ffc03e00 ffff8800e27e5040 ffff8800f00e86d8 [ 1.909055] ffff8800ffc03eb0 00000040ffffffff ffff8800f00e8000 00000000ffc03e30 [ 1.909055] Call Trace: [ 1.909055] <IRQ> [ 1.909055] [<ffffffff81066028>] ? pvclock_clocksource_read+0x58/0xd0 [ 1.909055] [<ffffffff81486352>] net_rx_action+0x112/0x240 [ 1.909055] [<ffffffff8107f319>] __do_softirq+0xb9/0x190 [ 1.909055] [<ffffffff815d8d7c>] call_softirq+0x1c/0x30 The problem is that the xenfront driver is passing a NULL page to __skb_fill_page_desc() which was unexpected. This patch checks that there is a page before dereferencing. Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Signed-off-by: Mel Gorman <mgorman@suse.de> --- include/linux/skbuff.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html