On Thu, Jan 13, 2022 at 09:12:32AM +0100, Christian Ehrhardt wrote:
>
> Hi all,
>
> On Wed, Jan 12, 2022 at 08:04:11PM +0100, Alexander Bluhm wrote:
> > On Wed, Jan 12, 2022 at 04:21:13PM +0100, Christian Ehrhardt wrote:
> > > > > - if (len <= tail - head) {
> > > > > - /* there's enough space in the first mbuf */
> > > > > -
> > > > > - if (len > tail - mtod(m0, caddr_t)) {
> > > > > + if (!M_READONLY(m0) && len <= tail - head) {
> > > > > + /* we can copy everything into the first mbuf */
> > > > > + if (m0->m_len == 0) {
> > > > > + m0->m_data = head;
> > > >
> > > > Before your patch, m_data was effectively set to head - adj in this
> > > > case,
> > > > because head += adj had not happened yet. Does this fix a bug in the old
> > > > code, or does it introduce a bug?
> > >
> > > Oh, you are right. If the first mbuf is empty but large enough
> > > to hold the pulled data, we previously did not honour the alignment.
> > > I admit that I didn't notice this semantic change but I still think
> > > that the new version of the code is what is supposed to happen.
> >
> > I think the new code preserves the alignment and that is better.
>
> Ok.
>
> > > > > + } else if (len > tail - mtod(m0, caddr_t)) {
> > > > > /* need to memmove to make space at the end */
> > > > > memmove(head, mtod(m0, caddr_t), m0->m_len);
> > > > > m0->m_data = head;
> > > > > + len -= m0->m_len;
> > > > > }
> > > > > -
> > > > > - len -= m0->m_len;
> >
> > I don't understand that chunk. Why do you move the "len -= m0->m_len" ?
> >
> > In the case "m0->m_len != 0" and "len <= tail - mtod(m0, caddr_t)"
> > "m0->m_len" octets are already at the correct place.
> > Only "len - m0->m_len" octets have to be copied from the following mbufs.
>
> You are right here. Somehow my mind skipped the condition on the
> else branch.
>
> > I think it should be:
> > [ ... ]
>
> True, updated patch (v3) below.
>
> regards Christian
>
>
> commit 2265c7db45a9127bcf236de6432d6dd323414bd5
> Author: Christian Ehrhardt <[email protected]>
> Date: Tue Jan 11 10:31:46 2022 +0100
>
> m_pullup: Properly handle read-only clusters
>
> If the first mbuf of a chain in m_pullup is a cluster, check if
> the cluster is read-only (shared or an external buffer). If so
> don't touch it an create an new mbuf for the pullup data.
One small comment below.
> diff --git a/sys/kern/uipc_mbuf.c b/sys/kern/uipc_mbuf.c
> index 5e4cb5ba88..21ae5059b0 100644
> --- a/sys/kern/uipc_mbuf.c
> +++ b/sys/kern/uipc_mbuf.c
> @@ -957,8 +957,6 @@ m_pullup(struct mbuf *m0, int len)
>
> head = M_DATABUF(m0);
> if (m0->m_len == 0) {
> - m0->m_data = head;
> -
> while (m->m_len == 0) {
> m = m_free(m);
> if (m == NULL)
> @@ -972,10 +970,11 @@ m_pullup(struct mbuf *m0, int len)
> tail = head + M_SIZE(m0);
> head += adj;
>
> - if (len <= tail - head) {
> - /* there's enough space in the first mbuf */
> -
> - if (len > tail - mtod(m0, caddr_t)) {
> + if (!M_READONLY(m0) && len <= tail - head) {
> + /* we can copy everything into the first mbuf */
> + if (m0->m_len == 0) {
> + m0->m_data = head;
> + } else if (len > tail - mtod(m0, caddr_t)) {
> /* need to memmove to make space at the end */
> memmove(head, mtod(m0, caddr_t), m0->m_len);
> m0->m_data = head;
> @@ -983,14 +982,18 @@ m_pullup(struct mbuf *m0, int len)
>
> len -= m0->m_len;
> } else {
> - /* the first mbuf is too small so make a new one */
> + /* the first mbuf is too small or read-only, make a new one */
> space = adj + len;
>
> if (space > MAXMCLBYTES)
> goto bad;
>
> - m0->m_next = m;
> - m = m0;
> + if (m0->m_len == 0) {
> + m_free(m0);
> + } else {
> + m0->m_next = m;
> + m = m0;
> + }
This change is not really needed. The for (;;) loop below that does the
copy will handle an empty initial mbuf just fine and m_free() it.
I would not change this since I think it makes the code more complex for
little gain.
>
> MGET(m0, M_DONTWAIT, m->m_type);
> if (m0 == NULL)
Diff is OK claudio@
--
:wq Claudio
