Double gnttab_end_access in mini-os netfront

Double gnttab_end_access in mini-os netfront

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2390 bytes --]

Hi,

When running HVM on Xen 4.6 with qemu in stubdom, I've found that
something goes wrong with disk frontend (at least that was visible
problem - a lot of read and write errors in stubdom log). But further
debugging (including -DGNT_DEBUG) leads to double gnttab_end_access in
netfront. 

The stack trace is:
ASSERTION FAILED: !(!inuse[ref]) at gnttab.c:42.
Do_exit called!
0x00000000000f3ffb: get_time_values_from_xen at xen-4.6.0/extras/mini-os/arch/x86/time.c:134 (discriminator 1)
0x00000000000d74a8: HYPERVISOR_sched_op at xen-4.6.0/extras/mini-os/include/x86/x86_64/hypercall-x86_64.h:180
0x00000000000d6a2e: put_free_entry at xen-4.6.0/extras/mini-os/gnttab.c:43
0x00000000000d6bff: gnttab_end_access at xen-4.6.0/extras/mini-os/gnttab.c:115
0x00000000000d8a50: network_rx at xen-4.6.0/extras/mini-os/netfront.c:134
0x00000000000d9ec4: netfront_receive at xen-4.6.0/extras/mini-os/netfront.c:671
0x00000000000dd302: get_current at xen-4.6.0/extras/mini-os/include/x86/arch_sched.h:16
0x0000000000079f72: tap_send at xen-4.6.0/stubdom/../tools/qemu-xen-traditional/net.c:756
0x00000000000069f9: main_loop_wait at xen-4.6.0/stubdom/../tools/qemu-xen-traditional/vl.c:3826
0x0000000000021f27: main_loop at xen-4.6.0/stubdom/../tools/qemu-xen-traditional/i386-dm/helper2.c:612 (discriminator 1)
0x000000000000950d: quit_timers at xen-4.6.0/stubdom/../tools/qemu-xen-traditional/vl.c:1866
0x00000000000d7f57: call_main at xen-4.6.0/extras/mini-os/main.c:163
0x0000000000003423: thread_starter at :?
0x0000000000000000: _start at ??:?

It was working in Xen 4.4. The only commit touching xenfront.c (in any
meaningful way) from that time is this one:
http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f348390652a67e9356eec9cd2b0f76a9c7c72

With that commit reverted, issue vanishes.

I guess it's because before this commit, there was "if (rx->status ==
NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)", but now
that case is handled after gnttab_end_access (using "if (rx->status >
NETIF_RSP_NULL)"). I think the fix would be to restore that "continue"
line.

PS What is the correct place for such reports? minios-devel? xen-devel?
both?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Double gnttab_end_access in mini-os netfront

[Ian Campbell]

On Sun, 2015-12-06 at 03:19 +0100, Marek Marczykowski-Górecki wrote:
> Hi,
> 
> When running HVM on Xen 4.6 with qemu in stubdom, I've found that
> something goes wrong with disk frontend (at least that was visible
> problem - a lot of read and write errors in stubdom log). But further
> debugging (including -DGNT_DEBUG) leads to double gnttab_end_access in
> netfront. 
> 
> The stack trace is:
> ASSERTION FAILED: !(!inuse[ref]) at gnttab.c:42.
> Do_exit called!
> 0x00000000000f3ffb: get_time_values_from_xen at xen-4.6.0/extras/mini-
> os/arch/x86/time.c:134 (discriminator 1)
> 0x00000000000d74a8: HYPERVISOR_sched_op at xen-4.6.0/extras/mini-
> os/include/x86/x86_64/hypercall-x86_64.h:180
> 0x00000000000d6a2e: put_free_entry at xen-4.6.0/extras/mini-
> os/gnttab.c:43
> 0x00000000000d6bff: gnttab_end_access at xen-4.6.0/extras/mini-
> os/gnttab.c:115
> 0x00000000000d8a50: network_rx at xen-4.6.0/extras/mini-os/netfront.c:134
> 0x00000000000d9ec4: netfront_receive at xen-4.6.0/extras/mini-
> os/netfront.c:671
> 0x00000000000dd302: get_current at xen-4.6.0/extras/mini-
> os/include/x86/arch_sched.h:16
> 0x0000000000079f72: tap_send at xen-4.6.0/stubdom/../tools/qemu-xen-
> traditional/net.c:756
> 0x00000000000069f9: main_loop_wait at xen-4.6.0/stubdom/../tools/qemu-
> xen-traditional/vl.c:3826
> 0x0000000000021f27: main_loop at xen-4.6.0/stubdom/../tools/qemu-xen-
> traditional/i386-dm/helper2.c:612 (discriminator 1)
> 0x000000000000950d: quit_timers at xen-4.6.0/stubdom/../tools/qemu-xen-
> traditional/vl.c:1866
> 0x00000000000d7f57: call_main at xen-4.6.0/extras/mini-os/main.c:163
> 0x0000000000003423: thread_starter at :?
> 0x0000000000000000: _start at ??:?
> 
> It was working in Xen 4.4. The only commit touching xenfront.c (in any
> meaningful way) from that time is this one:
> http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f348390652a67e
> 9356eec9cd2b0f76a9c7c72
> 
> With that commit reverted, issue vanishes.
> 
> I guess it's because before this commit, there was "if (rx->status ==
> NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)", but now
> that case is handled after gnttab_end_access (using "if (rx->status >
> NETIF_RSP_NULL)"). I think the fix would be to restore that "continue"
> line.

That sounds pretty plausible to me (FWIW). Have you tried it?

> PS What is the correct place for such reports? minios-devel? xen-devel?
> both?

Formally I suppose the former, but realistically including xen-devel as
well is likely to attract more eyes.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Double gnttab_end_access in mini-os netfront

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 3031 bytes --]

On Tue, Dec 08, 2015 at 11:33:49AM +0000, Ian Campbell wrote:
> On Sun, 2015-12-06 at 03:19 +0100, Marek Marczykowski-Górecki wrote:
> > Hi,
> > 
> > When running HVM on Xen 4.6 with qemu in stubdom, I've found that
> > something goes wrong with disk frontend (at least that was visible
> > problem - a lot of read and write errors in stubdom log). But further
> > debugging (including -DGNT_DEBUG) leads to double gnttab_end_access in
> > netfront. 
> > 
> > The stack trace is:
> > ASSERTION FAILED: !(!inuse[ref]) at gnttab.c:42.
> > Do_exit called!
> > 0x00000000000f3ffb: get_time_values_from_xen at xen-4.6.0/extras/mini-
> > os/arch/x86/time.c:134 (discriminator 1)
> > 0x00000000000d74a8: HYPERVISOR_sched_op at xen-4.6.0/extras/mini-
> > os/include/x86/x86_64/hypercall-x86_64.h:180
> > 0x00000000000d6a2e: put_free_entry at xen-4.6.0/extras/mini-
> > os/gnttab.c:43
> > 0x00000000000d6bff: gnttab_end_access at xen-4.6.0/extras/mini-
> > os/gnttab.c:115
> > 0x00000000000d8a50: network_rx at xen-4.6.0/extras/mini-os/netfront.c:134
> > 0x00000000000d9ec4: netfront_receive at xen-4.6.0/extras/mini-
> > os/netfront.c:671
> > 0x00000000000dd302: get_current at xen-4.6.0/extras/mini-
> > os/include/x86/arch_sched.h:16
> > 0x0000000000079f72: tap_send at xen-4.6.0/stubdom/../tools/qemu-xen-
> > traditional/net.c:756
> > 0x00000000000069f9: main_loop_wait at xen-4.6.0/stubdom/../tools/qemu-
> > xen-traditional/vl.c:3826
> > 0x0000000000021f27: main_loop at xen-4.6.0/stubdom/../tools/qemu-xen-
> > traditional/i386-dm/helper2.c:612 (discriminator 1)
> > 0x000000000000950d: quit_timers at xen-4.6.0/stubdom/../tools/qemu-xen-
> > traditional/vl.c:1866
> > 0x00000000000d7f57: call_main at xen-4.6.0/extras/mini-os/main.c:163
> > 0x0000000000003423: thread_starter at :?
> > 0x0000000000000000: _start at ??:?
> > 
> > It was working in Xen 4.4. The only commit touching xenfront.c (in any
> > meaningful way) from that time is this one:
> > http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f348390652a67e
> > 9356eec9cd2b0f76a9c7c72
> > 
> > With that commit reverted, issue vanishes.
> > 
> > I guess it's because before this commit, there was "if (rx->status ==
> > NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)", but now
> > that case is handled after gnttab_end_access (using "if (rx->status >
> > NETIF_RSP_NULL)"). I think the fix would be to restore that "continue"
> > line.
> 
> That sounds pretty plausible to me (FWIW). Have you tried it?

I've tried moving gnttab_end_access into that if branch. And it didn't
worked.

> > PS What is the correct place for such reports? minios-devel? xen-devel?
> > both?
> 
> Formally I suppose the former, but realistically including xen-devel as
> well is likely to attract more eyes.
> 
> Ian.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Double gnttab_end_access in mini-os netfront

[Samuel Thibault]

Marek Marczykowski-Górecki, on Tue 08 Dec 2015 12:46:31 +0100, wrote:
> > > http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f348390652a67e
> > > 9356eec9cd2b0f76a9c7c72
> > > 
> > > With that commit reverted, issue vanishes.
> > > 
> > > I guess it's because before this commit, there was "if (rx->status ==
> > > NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)", but now
> > > that case is handled after gnttab_end_access (using "if (rx->status >
> > > NETIF_RSP_NULL)"). I think the fix would be to restore that "continue"
> > > line.
> > 
> > That sounds pretty plausible to me (FWIW). Have you tried it?
> 
> I've tried moving gnttab_end_access into that if branch. And it didn't
> worked.

Which if branch?  Please show the code, C is less ambiguous than english
:)

Samuel

Re: [Minios-devel] Double gnttab_end_access in mini-os netfront

[Ian Campbell]

On Tue, 2015-12-08 at 13:00 +0100, Samuel Thibault wrote:
> Marek Marczykowski-Górecki, on Tue 08 Dec 2015 12:46:31 +0100, wrote:
> > > > http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f3483906
> > > > 52a67e
> > > > 9356eec9cd2b0f76a9c7c72
> > > > 
> > > > With that commit reverted, issue vanishes.
> > > > 
> > > > I guess it's because before this commit, there was "if (rx->status
> > > > ==
> > > > NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)",
> > > > but now
> > > > that case is handled after gnttab_end_access (using "if (rx->status 
> > > > >
> > > > NETIF_RSP_NULL)"). I think the fix would be to restore that
> > > > "continue"
> > > > line.
> > > 
> > > That sounds pretty plausible to me (FWIW). Have you tried it?
> > 
> > I've tried moving gnttab_end_access into that if branch. And it didn't
> > worked.
> 
> Which if branch?  Please show the code, C is less ambiguous than english
> :)

Details of in which way it didn't work would also be useful, I expect.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Minios-devel] Double gnttab_end_access in mini-os netfront

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2077 bytes --]

On Tue, Dec 08, 2015 at 12:11:34PM +0000, Ian Campbell wrote:
> On Tue, 2015-12-08 at 13:00 +0100, Samuel Thibault wrote:
> > Marek Marczykowski-Górecki, on Tue 08 Dec 2015 12:46:31 +0100, wrote:
> > > > > http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f3483906
> > > > > 52a67e
> > > > > 9356eec9cd2b0f76a9c7c72
> > > > > 
> > > > > With that commit reverted, issue vanishes.
> > > > > 
> > > > > I guess it's because before this commit, there was "if (rx->status
> > > > > ==
> > > > > NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)",
> > > > > but now
> > > > > that case is handled after gnttab_end_access (using "if (rx->status 
> > > > > >
> > > > > NETIF_RSP_NULL)"). I think the fix would be to restore that
> > > > > "continue"
> > > > > line.
> > > > 
> > > > That sounds pretty plausible to me (FWIW). Have you tried it?
> > > 
> > > I've tried moving gnttab_end_access into that if branch. And it didn't
> > > worked.
> > 
> > Which if branch?  Please show the code, C is less ambiguous than english
> > :)
> 
> Details of in which way it didn't work would also be useful, I expect.

I don't have my test environment handy, but the change was:
--- netfront.c.orig     2015-12-08 13:29:04.913000000 +0100
+++ netfront.c  2015-12-08 13:29:24.060000000 +0100
@@ -122,10 +122,10 @@
 
         buf = &dev->rx_buffers[id];
         page = (unsigned char*)buf->page;
-        gnttab_end_access(buf->gref);
 
         if (rx->status > NETIF_RSP_NULL)
         {
+        gnttab_end_access(buf->gref);
 #ifdef HAVE_LIBC
            if (dev->netif_rx == NETIF_SELECT_RX) {
                int len = rx->status;


But to be frank, I'm not entirely sure that the tested version was
really with this patch, as stubdom building code is quite convolved...
The crash was the same, but I'll test it again to be sure.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Double gnttab_end_access in mini-os netfront

[Samuel Thibault]

Ian Campbell, on Tue 08 Dec 2015 11:33:49 +0000, wrote:
> > http://xenbits.xen.org/gitweb/?p=mini-os.git;a=commit;h=7c8f348390652a67e
> > 9356eec9cd2b0f76a9c7c72
> > 
> > With that commit reverted, issue vanishes.
> > 
> > I guess it's because before this commit, there was "if (rx->status ==
> > NETIF_RSP_NULL) continue" before "gnttab_end_access(buf->gref)", but now
> > that case is handled after gnttab_end_access (using "if (rx->status >
> > NETIF_RSP_NULL)"). I think the fix would be to restore that "continue"
> > line.
> 
> That sounds pretty plausible to me (FWIW). Have you tried it?

That looks right, indeed.

Samuel