Re: Possible bug in m_split() when splitting M_EXT mbufs

2013-01-11 Thread Gleb Smirnoff
  Jacques,

On Fri, Jan 11, 2013 at 09:34:32AM +0200, Jacques Fourie wrote:
J Could someone please verify if m_split as in svn rev 245286 is doing the
J right thing in the scenario where a mbuf chain is split with len0 falling
J on a mbuf boundary and the mbuf in question being a M_EXT mbuf? Consider
J the following example where m0 is a mbuf chain consisting of 2 M_EXT mbufs,
J both 1448 bytes in length. Let len0 be 1448. The 'len0  m-m_len' check
J will be false so the for loop will not be entered in this case. We now have
J len = 1448 and remain = 0 and m still points to the first mbuf in the
J chain. Also assume that m0 is a pkthdr mbuf. A new pkthdr mbuf n will be
J allocated and initialized before the following piece of code is executed :
J 
J extpacket:
J if (m-m_flags  M_EXT) {
J n-m_data = m-m_data + len;
J mb_dupcl(n, m);
J } else {
J bcopy(mtod(m, caddr_t) + len, mtod(n, caddr_t), remain);
J }
J n-m_len = remain;
J m-m_len = len;
J n-m_next = m-m_next;
J m-m_next = NULL;
J return (n);
J 
J As m is a M_EXT mbuf the code in the if() clause will be executed. The
J problem is that m still points to the first mbuf so effectively the data
J pointer for n is assigned to the end of m's data pointer. It should
J actually point to the start of the data pointer of the next mbuf in the
J original m0 chain, right?

Thanks for analysis, Jacques.

IMHO, the following patch should suffice and won't break anything:

Index: uipc_mbuf.c
===
--- uipc_mbuf.c (revision 245223)
+++ uipc_mbuf.c (working copy)
@@ -1126,7 +1126,7 @@
u_int len = len0, remain;
 
MBUF_CHECKSLEEP(wait);
-   for (m = m0; m  len  m-m_len; m = m-m_next)
+   for (m = m0; m  len = m-m_len; m = m-m_next)
len -= m-m_len;
if (m == NULL)
return (NULL);

Can you please test it?

-- 
Totus tuus, Glebius.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: Inconsistent TCP state in tcp_do_segment()

2013-01-11 Thread Gleb Smirnoff
  Jacques,

On Fri, Jan 11, 2013 at 09:18:07AM +0200, Jacques Fourie wrote:
J I've been using the kernel socket API and noticed that every once in a
J while the data received at the remote end of a TCP connection doesn't match
J the data sent locally using sosend(). I tracked it down to the piece of
J code in tcp_do_segment() starting at line 2665: (svn rev 245286)
J 
Jif (acked  so-so_snd.sb_cc) {
J tp-snd_wnd -= so-so_snd.sb_cc;
J sbdrop_locked(so-so_snd, (int)so-so_snd.sb_cc);
J ourfinisacked = 1;
J } else {
J sbdrop_locked(so-so_snd, acked);
J tp-snd_wnd -= acked;
J ourfinisacked = 0;
J }
J sowwakeup_locked(so);
J /* Detect una wraparound. */
J if (!IN_RECOVERY(tp-t_flags) 
J SEQ_GT(tp-snd_una, tp-snd_recover) 
J SEQ_LEQ(th-th_ack, tp-snd_recover))
J tp-snd_recover = th-th_ack - 1;
J /* XXXLAS: Can this be moved up into cc_post_recovery? */
J if (IN_RECOVERY(tp-t_flags) 
J SEQ_GEQ(th-th_ack, tp-snd_recover)) {
J EXIT_RECOVERY(tp-t_flags);
J }
J tp-snd_una = th-th_ack;
J if (tp-t_flags  TF_SACK_PERMIT) {
J if (SEQ_GT(tp-snd_una, tp-snd_recover))
J tp-snd_recover = tp-snd_una;
J }
J if (SEQ_LT(tp-snd_nxt, tp-snd_una))
J tp-snd_nxt = tp-snd_una;
J 
J The issue is that sowwakeup_locked() is called before all the tcpcb fields
J are updated to account for the current ACK processing as can be seen from
J the tp-snd_una = th-th_ack in line 2686. My code that uses the kernel
J socket API calls sosend() in the upcall path resulting from
J sowwakeup_locked() which in turn can lead to tcp_output() being called with
J inconsistent TCP state. If I move the call to sowwakeup_locked() to after
J the 'if (SEQ_LT(tp-snd_nxt, tp-snd_una))' line in the code snippet above
J the data corruption issue seems to be fixed.

Again I think that your analysis is correct, thanks! I have added Robert
Watson to Cc of this email, so that he can review your suggestion.

However, it seems to me that it isn't safe to call sosend() from the
upcall directly. I suppose that if you run your code with WITNESS it will
report lock order reversals. The correct way for your module is to run
sosend() in separate context, for example using taskqueue(9) API, or
running separate thread for that.

-- 
Totus tuus, Glebius.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: Possible bug in m_split() when splitting M_EXT mbufs

2013-01-11 Thread Jacques Fourie
On Fri, Jan 11, 2013 at 10:12 AM, Gleb Smirnoff gleb...@freebsd.org wrote:

   Jacques,

 On Fri, Jan 11, 2013 at 09:34:32AM +0200, Jacques Fourie wrote:
 J Could someone please verify if m_split as in svn rev 245286 is doing the
 J right thing in the scenario where a mbuf chain is split with len0
 falling
 J on a mbuf boundary and the mbuf in question being a M_EXT mbuf? Consider
 J the following example where m0 is a mbuf chain consisting of 2 M_EXT
 mbufs,
 J both 1448 bytes in length. Let len0 be 1448. The 'len0  m-m_len' check
 J will be false so the for loop will not be entered in this case. We now
 have
 J len = 1448 and remain = 0 and m still points to the first mbuf in the
 J chain. Also assume that m0 is a pkthdr mbuf. A new pkthdr mbuf n will be
 J allocated and initialized before the following piece of code is
 executed :
 J
 J extpacket:
 J if (m-m_flags  M_EXT) {
 J n-m_data = m-m_data + len;
 J mb_dupcl(n, m);
 J } else {
 J bcopy(mtod(m, caddr_t) + len, mtod(n, caddr_t), remain);
 J }
 J n-m_len = remain;
 J m-m_len = len;
 J n-m_next = m-m_next;
 J m-m_next = NULL;
 J return (n);
 J
 J As m is a M_EXT mbuf the code in the if() clause will be executed. The
 J problem is that m still points to the first mbuf so effectively the data
 J pointer for n is assigned to the end of m's data pointer. It should
 J actually point to the start of the data pointer of the next mbuf in the
 J original m0 chain, right?

 Thanks for analysis, Jacques.

 IMHO, the following patch should suffice and won't break anything:

 Index: uipc_mbuf.c
 ===
 --- uipc_mbuf.c (revision 245223)
 +++ uipc_mbuf.c (working copy)
 @@ -1126,7 +1126,7 @@
 u_int len = len0, remain;

 MBUF_CHECKSLEEP(wait);
 -   for (m = m0; m  len  m-m_len; m = m-m_next)
 +   for (m = m0; m  len = m-m_len; m = m-m_next)
 len -= m-m_len;
 if (m == NULL)
 return (NULL);

 Can you please test it?


I think that your patch may cause other issues - m now points to the first
mbuf in the tail portion. The final piece of code under the extpacket:
label will then set m-m_len to 0 for example.




 --
 Totus tuus, Glebius.

___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: Possible bug in m_split() when splitting M_EXT mbufs

2013-01-11 Thread Jacques Fourie
On Fri, Jan 11, 2013 at 10:53 AM, Jacques Fourie
jacques.fou...@gmail.comwrote:




 On Fri, Jan 11, 2013 at 10:12 AM, Gleb Smirnoff gleb...@freebsd.orgwrote:

   Jacques,

 On Fri, Jan 11, 2013 at 09:34:32AM +0200, Jacques Fourie wrote:
 J Could someone please verify if m_split as in svn rev 245286 is doing
 the
 J right thing in the scenario where a mbuf chain is split with len0
 falling
 J on a mbuf boundary and the mbuf in question being a M_EXT mbuf?
 Consider
 J the following example where m0 is a mbuf chain consisting of 2 M_EXT
 mbufs,
 J both 1448 bytes in length. Let len0 be 1448. The 'len0  m-m_len'
 check
 J will be false so the for loop will not be entered in this case. We now
 have
 J len = 1448 and remain = 0 and m still points to the first mbuf in the
 J chain. Also assume that m0 is a pkthdr mbuf. A new pkthdr mbuf n will
 be
 J allocated and initialized before the following piece of code is
 executed :
 J
 J extpacket:
 J if (m-m_flags  M_EXT) {
 J n-m_data = m-m_data + len;
 J mb_dupcl(n, m);
 J } else {
 J bcopy(mtod(m, caddr_t) + len, mtod(n, caddr_t),
 remain);
 J }
 J n-m_len = remain;
 J m-m_len = len;
 J n-m_next = m-m_next;
 J m-m_next = NULL;
 J return (n);
 J
 J As m is a M_EXT mbuf the code in the if() clause will be executed. The
 J problem is that m still points to the first mbuf so effectively the
 data
 J pointer for n is assigned to the end of m's data pointer. It should
 J actually point to the start of the data pointer of the next mbuf in the
 J original m0 chain, right?

 Thanks for analysis, Jacques.

 IMHO, the following patch should suffice and won't break anything:

 Index: uipc_mbuf.c
 ===
 --- uipc_mbuf.c (revision 245223)
 +++ uipc_mbuf.c (working copy)
 @@ -1126,7 +1126,7 @@
 u_int len = len0, remain;

 MBUF_CHECKSLEEP(wait);
 -   for (m = m0; m  len  m-m_len; m = m-m_next)
 +   for (m = m0; m  len = m-m_len; m = m-m_next)
 len -= m-m_len;
 if (m == NULL)
 return (NULL);

 Can you please test it?


 I think that your patch may cause other issues - m now points to the first
 mbuf in the tail portion. The final piece of code under the extpacket:
 label will then set m-m_len to 0 for example.




 --
 Totus tuus, Glebius.


 I have been using the following patch that seems to fix the issue for me :

diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
index ab6163d..5c397fa 100644
--- a/sys/kern/uipc_mbuf.c
+++ b/sys/kern/uipc_mbuf.c
@@ -1132,6 +1132,23 @@ m_split(struct mbuf *m0, int len0, int wait)
return (NULL);
remain = m-m_len - len;
if (m0-m_flags  M_PKTHDR) {
+   if (remain == 0) {
+   if (m-m_next == NULL)
+   return (NULL);
+   if (!(m-m_next-m_flags  M_PKTHDR)) {
+   MGETHDR(n, wait, m0-m_type);
+   if (n == NULL)
+   return (NULL);
+   MH_ALIGN(n, 0);
+   n-m_next = m-m_next;
+   } else
+   n = m-m_next;
+   m-m_next = NULL;
+   n-m_pkthdr.rcvif = m0-m_pkthdr.rcvif;
+   n-m_pkthdr.len = m0-m_pkthdr.len - len0;
+   m0-m_pkthdr.len = len0;
+   return (n);
+   }
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


dirty hack asmc for Macbook 3,1

2013-01-11 Thread zeissoctopus
Dear All,

My dirty hack just works but not perfect.

--- /usr/src/sys/dev/asmc/asmcvar.h.original2013-01-11
09:36:53.0 +
+++ /usr/src/sys/dev/asmc/asmcvar.h 2013-01-11 10:21:02.0 +
@@ -141,10 +141,22 @@
 #define ASMC_MB_TEMPDESCS  { Enclosure Bottomside, \
  Northbridge Point 1, \
  Northbridge Point 2, Heatsink 1, \
  Heatsink 2, Memory Bank A, }
 
+#define ASMC_MB31_TEMPS { TB0T, TN0P, Th0H, Th1H, \
+  TM0P, NULL }
+
+#define ASMC_MB31_TEMPNAMES { enclosure, northbridge1, \
+  heatsink1, heatsink2,\
+  memory, }
+
+#define ASMC_MB31_TEMPDESCS { Enclosure Bottomside, \
+  Northbridge Point 1,  \
+  Heatsink 1, Heatsink2, \
+  Memory Bank A, }
+
 #define ASMC_MBP_TEMPS { TB0T, Th0H, Th1H, Tm0P, \
  TG0H, TG0P, TG0T, NULL }
 
 #define ASMC_MBP_TEMPNAMES { enclosure, heatsink1, \
  heatsink2, memory, graphics, \


--- /usr/src/sys/dev/asmc/asmc.c.original   2013-01-11
05:26:22.0 +
+++ /usr/src/sys/dev/asmc/asmc.c2013-01-11 09:51:01.0 +
@@ -153,10 +153,16 @@
  MacBook2,1, Apple SMC MacBook Core 2 Duo,
  ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, NULL, NULL, NULL,
  ASMC_MB_TEMPS, ASMC_MB_TEMPNAMES, ASMC_MB_TEMPDESCS
},
 
+{
+  MacBook3,1, Apple SMC MacBook Core 2 Duo,
+  ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, NULL, NULL, NULL,
+  ASMC_MB31_TEMPS, ASMC_MB31_TEMPNAMES, ASMC_MB31_TEMPDESCS
+},
+
{ 
  MacBookPro1,1, Apple SMC MacBook Pro Core Duo (15-inch),
  ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, ASMC_LIGHT_FUNCS,
  ASMC_MBP_TEMPS, ASMC_MBP_TEMPNAMES, ASMC_MBP_TEMPDESCS
},

The output of this new asmc on Macbook 3,1 :

dev.asmc.0.%desc: Apple SMC MacBook Core 2 Duo
dev.asmc.0.%driver: asmc
dev.asmc.0.%location: handle=\_SB_.PCI0.LPCB.SMC_
dev.asmc.0.%pnpinfo: _HID=APP0001 _UID=0
dev.asmc.0.%parent: acpi0
dev.asmc.0.fan.0.speed: 4535
dev.asmc.0.fan.0.safespeed: 0
dev.asmc.0.fan.0.minspeed: 1800
dev.asmc.0.fan.0.maxspeed: 6200
dev.asmc.0.fan.0.targetspeed: 4525
dev.asmc.0.temp.enclosure: 28
dev.asmc.0.temp.northbridge1: 52
dev.asmc.0.temp.heatsink1: 57
dev.asmc.0.temp.heatsink2: 55
dev.asmc.0.temp.memory: 51
dev.asmc.0.sms.x: -20
dev.asmc.0.sms.y: 45
dev.asmc.0.sms.z: 264

Regards,
zeissoctopus



--
View this message in context: 
http://freebsd.1045724.n5.nabble.com/dirty-hack-asmc-for-Macbook-3-1-tp5776597.html
Sent from the freebsd-hackers mailing list archive at Nabble.com.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Tracking FreeBSD development with SVNews

2013-01-11 Thread Larry Baird
This is a testimonial for a tool for tracking FreeBSD that doesn't get
enough publicity.  The tool is SVNnews.  Too see all FreeBSD modifications
for the last week try:
http://www.secnetix.de/olli/FreeBSD/svnews/

The real power is when you interested in looking at subsysystems.  

How about all of the modifications to FreeBSD 9 for the last week?
http://www.secnetix.de/olli/FreeBSD/svnews/?p=stable/9

How about all the FreeBSD 9 kernel modifications for the last week?
http://www.secnetix.de/olli/FreeBSD/svnews/?p=stable/9/sys

Hopefully you get the idea.  My thanks to Oliver for putting together
this useful tool.


-- 

Larry Baird
Global Technology Associates, Inc. 1992-2012| http://www.gta.com
Celebrating Twenty Years of Software Innovation | Orlando, FL
Email: l...@gta.com | TEL 407-380-0220
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: SATA disk disappears

2013-01-11 Thread Wojciech Puchar

GEOM_MIRROR: Component ada2 (device home1) broken, skipping.
GEOM_MIRROR: Cannot add disk ada2 to home1 (error=22).


started gmirror rebuild and it now works at full speed.

GEOM_MIRROR: Device home1: rebuilding provider ada2.


What kind of hardware failure may it be? smartctl -a /dev/ada2 shows disk 
is fine.


I had a new WD drive recently that had a write error.  Reallocated sector 
count did not go up, but it quickly failed the SMART self-test, short or 
long.  See smartctl(8) about the -t parameters.



this is WD green 3TB
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: SATA disk disappears

2013-01-11 Thread Warren Block

On Fri, 11 Jan 2013, Wojciech Puchar wrote:


GEOM_MIRROR: Component ada2 (device home1) broken, skipping.
GEOM_MIRROR: Cannot add disk ada2 to home1 (error=22).


started gmirror rebuild and it now works at full speed.

GEOM_MIRROR: Device home1: rebuilding provider ada2.


What kind of hardware failure may it be? smartctl -a /dev/ada2 shows disk 
is fine.


I had a new WD drive recently that had a write error.  Reallocated sector 
count did not go up, but it quickly failed the SMART self-test, short or 
long.  See smartctl(8) about the -t parameters.



this is WD green 3TB


1T Red here.  The firmware is likely very similar.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org


Re: dirty hack asmc for Macbook 3,1

2013-01-11 Thread Adrian Chadd
Please file a PR? :)


Thanks!



ADrian


On 11 January 2013 03:15, zeissoctopus cheungho...@gmail.com wrote:
 Dear All,

 My dirty hack just works but not perfect.

 --- /usr/src/sys/dev/asmc/asmcvar.h.original2013-01-11
 09:36:53.0 +
 +++ /usr/src/sys/dev/asmc/asmcvar.h 2013-01-11 10:21:02.0 +
 @@ -141,10 +141,22 @@
  #define ASMC_MB_TEMPDESCS  { Enclosure Bottomside, \
   Northbridge Point 1, \
   Northbridge Point 2, Heatsink 1, \
   Heatsink 2, Memory Bank A, }

 +#define ASMC_MB31_TEMPS { TB0T, TN0P, Th0H, Th1H, \
 +  TM0P, NULL }
 +
 +#define ASMC_MB31_TEMPNAMES { enclosure, northbridge1, \
 +  heatsink1, heatsink2,\
 +  memory, }
 +
 +#define ASMC_MB31_TEMPDESCS { Enclosure Bottomside, \
 +  Northbridge Point 1,  \
 +  Heatsink 1, Heatsink2, \
 +  Memory Bank A, }
 +
  #define ASMC_MBP_TEMPS { TB0T, Th0H, Th1H, Tm0P, \
   TG0H, TG0P, TG0T, NULL }

  #define ASMC_MBP_TEMPNAMES { enclosure, heatsink1, \
   heatsink2, memory, graphics, \


 --- /usr/src/sys/dev/asmc/asmc.c.original   2013-01-11
 05:26:22.0 +
 +++ /usr/src/sys/dev/asmc/asmc.c2013-01-11 09:51:01.0 +
 @@ -153,10 +153,16 @@
   MacBook2,1, Apple SMC MacBook Core 2 Duo,
   ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, NULL, NULL, NULL,
   ASMC_MB_TEMPS, ASMC_MB_TEMPNAMES, ASMC_MB_TEMPDESCS
 },

 +{
 +  MacBook3,1, Apple SMC MacBook Core 2 Duo,
 +  ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, NULL, NULL, NULL,
 +  ASMC_MB31_TEMPS, ASMC_MB31_TEMPNAMES, ASMC_MB31_TEMPDESCS
 +},
 +
 {
   MacBookPro1,1, Apple SMC MacBook Pro Core Duo (15-inch),
   ASMC_SMS_FUNCS, ASMC_FAN_FUNCS, ASMC_LIGHT_FUNCS,
   ASMC_MBP_TEMPS, ASMC_MBP_TEMPNAMES, ASMC_MBP_TEMPDESCS
 },

 The output of this new asmc on Macbook 3,1 :

 dev.asmc.0.%desc: Apple SMC MacBook Core 2 Duo
 dev.asmc.0.%driver: asmc
 dev.asmc.0.%location: handle=\_SB_.PCI0.LPCB.SMC_
 dev.asmc.0.%pnpinfo: _HID=APP0001 _UID=0
 dev.asmc.0.%parent: acpi0
 dev.asmc.0.fan.0.speed: 4535
 dev.asmc.0.fan.0.safespeed: 0
 dev.asmc.0.fan.0.minspeed: 1800
 dev.asmc.0.fan.0.maxspeed: 6200
 dev.asmc.0.fan.0.targetspeed: 4525
 dev.asmc.0.temp.enclosure: 28
 dev.asmc.0.temp.northbridge1: 52
 dev.asmc.0.temp.heatsink1: 57
 dev.asmc.0.temp.heatsink2: 55
 dev.asmc.0.temp.memory: 51
 dev.asmc.0.sms.x: -20
 dev.asmc.0.sms.y: 45
 dev.asmc.0.sms.z: 264

 Regards,
 zeissoctopus



 --
 View this message in context: 
 http://freebsd.1045724.n5.nabble.com/dirty-hack-asmc-for-Macbook-3-1-tp5776597.html
 Sent from the freebsd-hackers mailing list archive at Nabble.com.
 ___
 freebsd-hackers@freebsd.org mailing list
 http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
 To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org