On Thu, Jul 08, 2010 at 01:58:50PM +0200, Andre Oppermann wrote:
> On 08.07.2010 13:47, Kostik Belousov wrote:
> >On Thu, Jul 08, 2010 at 01:34:28PM +0200, Andre Oppermann wrote:
> >>On 08.07.2010 11:42, Kostik Belousov wrote:
> >>>On Thu, Jul 08, 2010 at 11:40:05AM +0300, Andriy Gapon wrote:
> >>>>on 08/07/2010 11:29 Kostik Belousov said the following:
> >>>>>Right, the patch maps the page in sf buffer read-only (on i386 only).
> >>>>>But note the parallel posting with m_cat() change. It is still not
> >>>>>enough,
> >>>>>and I am not set up for the real network testing ATM.
> >>>>
> >>>>Could you also try to experiment with mb_dupcl?
> >>>>Namely transfer M_RDONLY from source mbuf.
> >>>
> >>>Right, it is it.
> >>>
> >>>Below is my current patch including debugging facilities that seems to
> >>>work.
> >>>Real changes that needed are in m_cat and mb_dupcl.
> >>>
> >>...
> >>>diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
> >>>index f41eb03..1701ef2 100644
> >>>--- a/sys/kern/uipc_mbuf.c
> >>>+++ b/sys/kern/uipc_mbuf.c
> >>>@@ -301,7 +301,7 @@ mb_dupcl(struct mbuf *n, struct mbuf *m)
> >>> n->m_ext.ext_size = m->m_ext.ext_size;
> >>> n->m_ext.ref_cnt = m->m_ext.ref_cnt;
> >>> n->m_ext.ext_type = m->m_ext.ext_type;
> >>>- n->m_flags |= M_EXT;
> >>>+ n->m_flags |= M_EXT | (M_RDONLY& m->m_flags);
> >>> }
> >>
> >>Having the M_EXT flag always implies readonly and M_WRITABLE gets this
> >>right. Not inheriting all the flags from the source seems questionable.
> >>So IMHO this should be done here:
> >>
> >> n->m_flags |= (M_EXT | m->m_flags)
> >>
> >>> /*
> >>>@@ -911,7 +911,8 @@ m_cat(struct mbuf *m, struct mbuf *n)
> >>> m = m->m_next;
> >>> while (n) {
> >>> if (m->m_flags& M_EXT ||
> >>>- m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN]) {
> >>>+ m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN] ||
> >>>+ !M_WRITABLE(m)) {
> >>
> >>Here you can fully replace the (m->m_flags& M_EXT) test with
> >>M_WRITABLE(). The M_EXT test is included in it.
> >>
> >>> /* just join the two chains */
> >>> m->m_next = n;
> >>> return;
> >
> >The patch is below. Works as well for me, thank you for the feedback.
>
> You may want to run the change to m_dupcl() by rwatson just to be sure.
> It should be the right thing to do but better have another mbuf expert
> look at it.
Added Robert to Cc: list, let's see.
>
> Does this fix the sendfile corruption problem for all cases or just the
> panic for the loopback case?
I do not know. It fixes both panics with debugging patches applied and
indeed prevents corruption, for my setup on loopback. I have to defer to
external testing for !lo case.
>
> >diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
> >index f41eb03..58567a4 100644
> >--- a/sys/kern/uipc_mbuf.c
> >+++ b/sys/kern/uipc_mbuf.c
> >@@ -301,7 +301,7 @@ mb_dupcl(struct mbuf *n, struct mbuf *m)
> > n->m_ext.ext_size = m->m_ext.ext_size;
> > n->m_ext.ref_cnt = m->m_ext.ref_cnt;
> > n->m_ext.ext_type = m->m_ext.ext_type;
> >- n->m_flags |= M_EXT;
> >+ n->m_flags |= M_EXT | m->m_flags;
> > }
> >
> > /*
> >@@ -910,7 +910,7 @@ m_cat(struct mbuf *m, struct mbuf *n)
> > while (m->m_next)
> > m = m->m_next;
> > while (n) {
> >- if (m->m_flags& M_EXT ||
> >+ if (!M_WRITABLE(m) ||
> > m->m_data + m->m_len + n->m_len>=&m->m_dat[MLEN]) {
> > /* just join the two chains */
> > m->m_next = n;
>
> --
> Andre
pgpSt9RR4GOeT.pgp
Description: PGP signature
