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.
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;
+ }
MGET(m0, M_DONTWAIT, m->m_type);
if (m0 == NULL)
smime.p7s
Description: S/MIME cryptographic signature
