Date: Thu, 8 Feb 2001 22:09:28 -0500 From: "Bosko Milekic" <bmilekic@technokratis.com> To: "Boris Popov" <bp@butya.kz> Cc: <freebsd-arch@FreeBSD.ORG>, <freebsd-net@FreeBSD.ORG> Subject: Re: Sequential mbuf read/write extensions Message-ID: <001301c09245$b7400a00$1f90c918@jehovah> References: <Pine.BSF.4.21.0102090831010.24710-100000@lion.butya.kz>
next in thread | previous in thread | raw e-mail | index | archive | help
Boris Popov wrote:
[...]
> > For this to work, though, m_getm() needs to be modified to free
all of
> > `top' chain if it can't get either a cluster or an mbuf. I don't
know
> > if this was intentional, but it seems to me that there is a subtle
> > problem in m_getm() as it is now:
> >
> > if (len > MINCLSIZE) {
> > MCLGET(m, M_TRYWAIT);
> > if ((m->m_flags & M_EXT) == 0) {
> > m_freem(m); <------ frees only one mbuf
> ^^^^^^^^^^ cluster is not in the chain yet, so it have to be
> freed.
m_free() may be more appropriate than m_freem() then, but see
below.
> > return NULL;
> > }
> > }
> >
> > I think what may happen here is that you will leak your `top'
chain if
> > you fail to allocate a cluster.
>
> The original semantic was not to free an entire chain because
> m_getm() do not reallocates original (top) mbuf(s) (which may
contain
> data) and only adds new mbufs/clusters if possible. So, the calls
like
> m_get(mb->mb_top) will not left the wild pointer. There is also
simple way
> to deal with such behavior:
>
> mtop = m_get(...);
> if (mtop == NULL)
> fail;
> if (m_getm(mtop) == NULL) {
> m_freem(mtop);
> fail;
> }
>
> Probably m_getm() should return error code rather than pointer to
> mbuf to avoid confusion.
I understand this part, but what I think you missed in my comment
is that m_getm() should probably free what it already allocated before
finally failing. It may not need to free `top' because of the wild
pointer, as you say. But think of this:
m_getm() is called with a larger `size' - it decides that given the
`size' it will need to allocate a total of exactly 6 mbufs and 6
clusters for each mbuf. It loops and allocates, succesfully, 5 of
those mbufs and 5 clusters. So `top' chain has now grown and includes
those mbufs. Then what happens in the last iteration is that it
allocates the 6th mbuf OK (it has not yet placed it on the chain) and
fails to allocate a cluster, so it frees just that one mbuf (and not
the mbufs it allocated in prior iterations and attached to `top'
chain) and returns NULL. Your code that calls m_getm() then just
fails, leaving `top' with what it could allocate. Note that in my mail
I said "assuming this is a leak," thus recognizing the possibility
that you did this intentionally. :-) Right now, I'll assume that this
_was_ intentional, as that is what I understand from the above. But in
any case, if we do move this to uipc_mbuf.c, we need to do one of the
following:
(a) make m_getm() free what it allocated in previous loop iterations
before it failed (as described above) or
(b) leave m_getm() the way it is BUT write an additional function that
will simply wrap the call to m_getm() and flush properly for it if it
fails (EXACTLY like your code snippet above).
I'll gladly settle for either, but if we do go with (b), then the
m_freem() should be changed to an m_free(), as it reflects the fact
that we are only freeing the one mbuf and we should document this
behavior, certainly. If you want, I'll roll up a diff in a few days
(once I get what is presently dragging in my "commit this" queue out)
and commit it. If you prefer to do this yourself, then feel free. :-)
> --
> Boris Popov
> http://www.butya.kz/~bp/
Regards,
Bosko.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?001301c09245$b7400a00$1f90c918>