From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 783E35A0274 for ; Mon, 12 Feb 2024 00:18:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1707693520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kNGepkUtkbEBHnT7xsNsSneKEr6Yy0c5Hk5mbh62tU4=; b=GHwUrj8EpoOHCeOAh3z4Js/1X4QwQ6mKe8n42mbWBQgY5voFTlXWc5XN87xru+D6vxDu80 cv7ZJVmU08d4mWPTnwzGCTzhUmpRTmympgMIEUNzmV104fmM3ZJJRy7ooqRC9JZ+9L6HIU ElF50KwYo8BZq4WGA8KbKGaqW/t9AWU= Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-133-vY-DmY9LN8qdaEszIXy-dw-1; Sun, 11 Feb 2024 18:18:38 -0500 X-MC-Unique: vY-DmY9LN8qdaEszIXy-dw-1 Received: by mail-lf1-f69.google.com with SMTP id 2adb3069b0e04-51144722c56so2680337e87.2 for ; Sun, 11 Feb 2024 15:18:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707693517; x=1708298317; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=kNGepkUtkbEBHnT7xsNsSneKEr6Yy0c5Hk5mbh62tU4=; b=s19VHwQ3giyT8/s5kxL255z0TDq6Ef8rhkyr7BQso7xx6Y67giZsuF8es+d/y0u4Nu IUmr2lhgzoBfoc5loGUak9pY68rmr2aCeAOTZYcCoaepB8PIDJxtf9jq4ddJ/HU6iXsw ZQN17uPUbbprVrJui/4SEM3NLcnUDGncjqS/fXjWDvheCAICLjyVb34CkUpaEGBlmQmO gmaCgWKl3qJ7b3+S8lG79mvp8rOOmro3QWvObFcvOJ/KiakDqjJrwBVxYjp5rHMqMLI3 LJv0OszNmoCqPC5bWz/AyubtNwRcoKO4cyxybKD1shqZDip338euhVBQgXCe/DCUONfm BrUg== X-Gm-Message-State: AOJu0YwNOXI3/9dViFfsMIOV5KHinQmPqGhls6Wrav4TPIen7E1tmQEk D4ONds8n2fg6dkfzSxsBMXe6bBCMnNZMFla4BiVfBYOyskVPjJ5Z3GggVjMNBne8pANy+FRyKNw beqXOdZ9RNRdUZDUXLog+rOTZnPCZeItYVUrwCDu4b5Coh7yxkQ== X-Received: by 2002:a05:6512:15a0:b0:511:7a31:d66d with SMTP id bp32-20020a05651215a000b005117a31d66dmr4045522lfb.57.1707693516900; Sun, 11 Feb 2024 15:18:36 -0800 (PST) X-Google-Smtp-Source: AGHT+IHSVSSO6iggEN38htMkjpDvx5Z9GpMFVFzM3gBeOyWJwGGe3GPqhpoP85VH1DRLo0/WLMoDFA== X-Received: by 2002:a05:6512:15a0:b0:511:7a31:d66d with SMTP id bp32-20020a05651215a000b005117a31d66dmr4045515lfb.57.1707693516453; Sun, 11 Feb 2024 15:18:36 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCV0BUXtvRPcDEzjxvunJ8upMvlJOaqRX9+WQ3lqk1muXEABmILEdMZpzsK+DfAsm3AdLrDr5iH3QeFh4Idrn3P2TxtQ Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id ig4-20020a056402458400b0056098a293cdsm2142860edb.69.2024.02.11.15.18.35 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 11 Feb 2024 15:18:35 -0800 (PST) Date: Mon, 12 Feb 2024 00:18:02 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 16/24] packet: replace struct desc by struct iovec Message-ID: <20240212001802.40c4f23e@elisabeth> In-Reply-To: References: <20240202141151.3762941-1-lvivier@redhat.com> <20240202141151.3762941-17-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.1.1 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: DIWWT3ZMUXVVJFMQU4THOAC4N4T6RCMH X-Message-ID-Hash: DIWWT3ZMUXVVJFMQU4THOAC4N4T6RCMH X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Laurent Vivier , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 6 Feb 2024 13:25:18 +1100 David Gibson wrote: > On Fri, Feb 02, 2024 at 03:11:43PM +0100, Laurent Vivier wrote: > > Rationale please. It's probably also worth nothing that this does > replace struct desc with a larger structure - struct desc is already > padded out to 8 bytes, but on 64-bit machines iovec will be larger > still. Right, yes, that becomes 16 bytes (from 8), and those arrays are already quite large. I wonder if we can keep struct desc, but I have no idea how complicated it is. > > Signed-off-by: Laurent Vivier > > --- > > packet.c | 75 +++++++++++++++++++++++++++++++------------------------- > > packet.h | 14 ++--------- > > 2 files changed, 43 insertions(+), 46 deletions(-) > > > > diff --git a/packet.c b/packet.c > > index ccfc84607709..af2a539a1794 100644 > > --- a/packet.c > > +++ b/packet.c > > @@ -22,6 +22,36 @@ > > #include "util.h" > > #include "log.h" > > > > +static int packet_check_range(const struct pool *p, size_t offset, size_t len, > > + const char *start, const char *func, int line) > > +{ > > + if (start < p->buf) { > > + if (func) { > > + trace("add packet start %p before buffer start %p, " > > + "%s:%i", (void *)start, (void *)p->buf, func, line); > > + } > > + return -1; > > I guess not really in scope for this patch, but IIUC the only place > we'd hit this is if the caller has done something badly wrong, so > possibly it should be an ASSERT(). I wouldn't terminate passt in any of these cases, because that can turn a safety check into a possibility for a denial of service. These checks are here not to avoid obvious logic issues, but rather to make them less harmful if present. In general, we assume that the hypervisor is able to wreck its own connectivity, but we should make it harder for users in the guest to do so. > > + } > > + > > + if (start + len + offset > p->buf + p->buf_size) { > > + if (func) { > > + trace("packet offset plus length %lu from size %lu, " > > + "%s:%i", start - p->buf + len + offset, > > + p->buf_size, func, line); > > + } > > + return -1; > > + } > > Same with this one. > > > + > > +#if UINTPTR_MAX == UINT64_MAX > > + if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { > > + trace("add packet start %p, buffer start %p, %s:%i", > > + (void *)start, (void *)p->buf, func, line); > > + return -1; > > + } > > +#endif > > This one is relevant to this patch though - because you're expanding > struct desc's 32-bit offset to full void * from struct iovec, this > check is no longer relevant. > > > + > > + return 0; > > +} > > /** > > * packet_add_do() - Add data as packet descriptor to given pool > > * @p: Existing pool > > @@ -41,34 +71,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start, > > return; > > } > > > > - if (start < p->buf) { > > - trace("add packet start %p before buffer start %p, %s:%i", > > - (void *)start, (void *)p->buf, func, line); > > + if (packet_check_range(p, 0, len, start, func, line)) > > return; > > - } > > - > > - if (start + len > p->buf + p->buf_size) { > > - trace("add packet start %p, length: %zu, buffer end %p, %s:%i", > > - (void *)start, len, (void *)(p->buf + p->buf_size), > > - func, line); > > - return; > > - } > > > > if (len > UINT16_MAX) { > > trace("add packet length %zu, %s:%i", len, func, line); > > return; > > } > > > > -#if UINTPTR_MAX == UINT64_MAX > > - if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) { > > - trace("add packet start %p, buffer start %p, %s:%i", > > - (void *)start, (void *)p->buf, func, line); > > - return; > > - } > > -#endif > > - > > - p->pkt[idx].offset = start - p->buf; > > - p->pkt[idx].len = len; > > + p->pkt[idx].iov_base = (void *)start; > > + p->pkt[idx].iov_len = len; > > > > p->count++; > > } > > @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset, > > return NULL; > > } > > > > - if (p->pkt[idx].offset + len + offset > p->buf_size) { > > + if (len + offset > p->pkt[idx].iov_len) { > > if (func) { > > - trace("packet offset plus length %zu from size %zu, " > > - "%s:%i", p->pkt[idx].offset + len + offset, > > - p->buf_size, func, line); > > + trace("data length %zu, offset %zu from length %zu, " > > + "%s:%i", len, offset, p->pkt[idx].iov_len, > > + func, line); > > } > > return NULL; > > } > > > > - if (len + offset > p->pkt[idx].len) { > > - if (func) { > > - trace("data length %zu, offset %zu from length %u, " > > - "%s:%i", len, offset, p->pkt[idx].len, > > - func, line); > > - } > > + if (packet_check_range(p, offset, len, p->pkt[idx].iov_base, > > + func, line)) > > return NULL; > > - } > > > > if (left) > > - *left = p->pkt[idx].len - offset - len; > > + *left = p->pkt[idx].iov_len - offset - len; > > > > - return p->buf + p->pkt[idx].offset + offset; > > + return (char *)p->pkt[idx].iov_base + offset; > > } > > > > /** > > diff --git a/packet.h b/packet.h > > index a784b07bbed5..8377dcf678bb 100644 > > --- a/packet.h > > +++ b/packet.h > > @@ -6,16 +6,6 @@ > > #ifndef PACKET_H > > #define PACKET_H > > > > -/** > > - * struct desc - Generic offset-based descriptor within buffer > > - * @offset: Offset of descriptor relative to buffer start, 32-bit limit > > - * @len: Length of descriptor, host order, 16-bit limit > > - */ > > -struct desc { > > - uint32_t offset; > > - uint16_t len; > > -}; > > - > > /** > > * struct pool - Generic pool of packets stored in a buffer > > * @buf: Buffer storing packet descriptors > > @@ -29,7 +19,7 @@ struct pool { > > size_t buf_size; > > size_t size; > > size_t count; > > - struct desc pkt[1]; > > + struct iovec pkt[1]; > > }; > > > > void packet_add_do(struct pool *p, size_t len, const char *start, > > @@ -54,7 +44,7 @@ struct _name ## _t { \ > > size_t buf_size; \ > > size_t size; \ > > size_t count; \ > > - struct desc pkt[_size]; \ > > + struct iovec pkt[_size]; \ > > } > > > > #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size) \ > -- Stefano