On Mon, 25 May 2026 15:09:54 +0200 Stefano Garzarella <[email protected]> wrote:
> On Mon, May 25, 2026 at 08:42:01AM -0400, Michael S. Tsirkin wrote: > >On Mon, May 25, 2026 at 11:53:14AM +0100, David Laight wrote: > >> On Mon, 25 May 2026 11:57:45 +0200 > >> Stefano Garzarella <[email protected]> wrote: > >> > >> > On Sat, May 23, 2026 at 05:35:57PM +0100, David Laight wrote: > >> > >On Sat, 23 May 2026 02:20:29 +0000 > >> > >[email protected] wrote: > >> > > > >> > >> Hello: > >> > >> > >> > >> This patch was applied to netdev/net.git (main) > >> > >> by Jakub Kicinski <[email protected]>: > >> > > > >> > >Did anyone else notice that is isn't a bug? > >> > > > >> > >There is no way that a 'count of bytes of kernel memory' can overflow > >> > >the size of 'long'. > >> > > >> > It's more of an estimate than an actual calculation of memory usage if > >> > we queue the incoming packet. In theory, an overflow could occur if the > >> > user sets `buf_alloc` to 4GB. In practice, though, I think you're right: > >> > the memory should run out before we get to that check. > >> > >> The calculation is: > >> > >> u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * > >> SKB_TRUESIZE(0); > >> > >> skb_queue_len() will be the number of items on the queue. > >> SKB_TRUESIZE(0) is the memory taken up by a zero length skb (basically > >> sizeof(skb)). > >> > >> Unless you either corrupt the queue length or manage to allocate skb that > >> use > >> less than the minimum about of memory that product can't overflow > >> 'unsigned long'. > >> > >> The later calculations might wrap - but the multiply can't. > >> > >> -- David > > > > > >Indeed, I wasn't thinking. For this to even get close to overflowing > >we'd have to have almost all of 4G available to the 32 bit kernel taken > >up by this single queue. Except there is usually only 1G or 2G available to the kernel. And all the skb would have to contain no data. > > > >Revert, I'd say. > > I also blindly added the cast to silence sashiko :-( > I see now that it could never actually happen, but semantically it’s > correct, so maybe we can avoid the revert. Lots of things are semantically correct :-) I didn't look any further down the function to see if it could be 'unsigned long' (or even size_t - but I like 'proper' types when they are always correct, I have to remember that size_t is unsigned long). The problem with the (u64) cast is that gcc is very likely to make a 'pigs breakfast' of it and do a full 64x64 multiply. It'll then try to keep the 64bit value in a register-pair which ends up being spilled to stack as a pair. I've seen it spill a constant zero and do a multiply by an immediate zero when doing 64bit maths on 32bit x86. I think gcc can hold a 64bit value as two separate 32bit values; that can generate reasonable code. But if they get merged (eg because of an "=A" asm constraint) it all goes horribly wrong. This is why there are some asm 'helpers' for mixed 32bit/64bit maths. -- David > > Thanks, > Stefano > > > > >> > > >> > Thanks, > >> > Stefano > >> > > >> > > > >> > >-- David > >> > > > >> > >> > >> > >> On Thu, 21 May 2026 14:47:32 +0200 you wrote: > >> > >> > From: Stefano Garzarella <[email protected]> > >> > >> > > >> > >> > On 32-bit architectures, both skb_queue_len() and SKB_TRUESIZE(0) > >> > >> > evaluate > >> > >> > to 32-bit values. The multiplication can overflow before being > >> > >> > assigned to > >> > >> > the u64 skb_overhead variable, making the skb overhead check > >> > >> > ineffective. > >> > >> > > >> > >> > Cast skb_queue_len() to u64 so the multiplication is always > >> > >> > performed in > >> > >> > 64-bit arithmetic. > >> > >> > > >> > >> > [...] > >> > >> > >> > >> Here is the summary with links: > >> > >> - [net] vsock/virtio: fix skb overhead overflow on 32-bit builds > >> > >> https://git.kernel.org/netdev/net/c/4157501b9a8f > >> > >> > >> > >> You are awesome, thank you! > >> > > > >> > > > >
