Date: Fri, 31 May 2002 16:23:57 -0400 From: Bosko Milekic <bmilekic@unixdaemons.com> To: Julian Elischer <julian@elischer.org> Cc: Archie Cobbs <archie@dellroad.org>, freebsd-net@FreeBSD.ORG Subject: Re: m_split() considered harmful Message-ID: <20020531162357.A71512@unixdaemons.com> In-Reply-To: <Pine.BSF.4.21.0205311243460.29361-100000@InterJet.elischer.org>; from julian@elischer.org on Fri, May 31, 2002 at 12:54:39PM -0700 References: <20020531145938.A71219@unixdaemons.com> <Pine.BSF.4.21.0205311243460.29361-100000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, May 31, 2002 at 12:54:39PM -0700, Julian Elischer wrote: > > > On Fri, 31 May 2002, Bosko Milekic wrote: > > > > > I don't remember why the ext_size here was this originally (as you > > mention, it was imported that way), but it certainly seems bogus and > > you catching it now is hopefully going to solve some really wierd and > > difficult-to-debug problems we've had involving mbufs over the years. > > > It was imported from FreeBSD-1.x by rod. > > The size argument was for two reasons. > 1/ to allow the M_LEADINGSPACE and M_TRAILINGSPACE calculations > to be done if it was a single reference object. > 2/ to allow the free function (in the case of non cluster external > objects) to know what sized object they had in the case that they needed > this information. Yeah, we still need the field to exist because of (2). I just meant that we should remove the explicit setting of 0 of the field in m_split(), and that's what Archie suggested. > I know because I added it, because I needed to do both of these at TRW in > '90-'95 under MACH/BSD and when I moved the code to FreeBSD1.x it cam > along.. there was no M_LEADINGSPACE/M_TRAILINGSPACE macro at that time.. > I did it in my code explicitly. > > It was not used in standard code because only in my own protocol stack > did I know that the damned object was not shared and writable.. > This has improved with time.. > > Having the size set to 0 however stopped users from hitting > cases where the WRITABILITY of the ext objext has not been correctly > tracked as it always returns "not enough space" (sometimes -ve). Right now, in -CURRENT, we have a M_WRITABLE macro, as you noticed, that evalutes true unless one of these conditions is true: 1) M_RDONLY is set in the mbuf 2) mbuf has external buffer attached and ref. count on that buffer (e.g., cluster, whatever) is > 1. This macro (M_WRITABLE) _is_ currently used in M_LEADINGSPACE. If the mbuf being checked is not writable, M_LEADINGSPACE will evalute to 0. However, I noticed just now that it is not used in M_TRAILINGSPACE, where it probably should be. M_TRAILINGSPACE should probably look like this: #define M_TRAILINGSPACE(m) \ ((m)->m_flags & M_EXT ? \ (M_WRITABLE(m) ? \ (m)->m_ext.ext_buf + (m)->m_ext.ext_size - \ ((m)->m_data + (m)->m_len) : 0) : \ &(m)->m_dat[MLEN] - ((m)->m_data + (m)->m_len)) This is the same philosophy adopted by M_LEADINGSPACE. I have no idea how I missed that when we did the M_WRITABLE stuff. Or did we leave it out for some specific reason? Ian Dowse or David Malone would know, as the WRITABLE code was thanks to them. :-) > If we change this we need to audit the WRITABILTY.. > e.g. it is not checked in M_TRAILINGSPACE Right. > Julian -- Bosko Milekic bmilekic@unixdaemons.com bmilekic@FreeBSD.org To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020531162357.A71512>