| Message ID | 20241113201631.2920541-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/2] migration: Check current_migration in migration_is_running() | expand |
Hi, On 13/11/24 20:16, Peter Xu wrote: > From: Dmitry Frolov <frolov@swemel.ru> > > stat64_add() takes uint64_t as 2nd argument, but both > "p->next_packet_size" and "p->packet_len" are uint32_t. > Thus, theyr sum may overflow uint32_t. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru> > Link: https://lore.kernel.org/r/20241113140509.325732-2-frolov@swemel.ru > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/multifd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 4374e14a96..498e71fd10 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque) > } > > stat64_add(&mig_stats.multifd_bytes, > - p->next_packet_size + p->packet_len); > + (uint64_t)p->next_packet_size + p->packet_len); I am not familiar with this area, but quickly looking I can't find a code path accepting 4GiB payload, so IMHO this hypothetical case is not unreachable. My 2 cents (I'm not objecting on this "silence this warning" patch). > > p->next_packet_size = 0; > multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE);
On Wed, Nov 13, 2024 at 09:40:50PM +0100, Philippe Mathieu-Daudé wrote: > Hi, > > On 13/11/24 20:16, Peter Xu wrote: > > From: Dmitry Frolov <frolov@swemel.ru> > > > > stat64_add() takes uint64_t as 2nd argument, but both > > "p->next_packet_size" and "p->packet_len" are uint32_t. > > Thus, theyr sum may overflow uint32_t. > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru> > > Link: https://lore.kernel.org/r/20241113140509.325732-2-frolov@swemel.ru > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/multifd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/migration/multifd.c b/migration/multifd.c > > index 4374e14a96..498e71fd10 100644 > > --- a/migration/multifd.c > > +++ b/migration/multifd.c > > @@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque) > > } > > stat64_add(&mig_stats.multifd_bytes, > > - p->next_packet_size + p->packet_len); > > + (uint64_t)p->next_packet_size + p->packet_len); > > I am not familiar with this area, but quickly looking I can't > find a code path accepting 4GiB payload, so IMHO this hypothetical > case is not unreachable. My 2 cents (I'm not objecting on this > "silence this warning" patch). Thanks Phil, for taking an extra eye. Yes, the solo goal is probably to silent it, if that helps anyone at all. I left similar comment when replying to Dmitry when queuing this. If it could overflow, we have more troubles, e.g., we have plenty of places caching these values in 32bits. When this overflow could happen, we should simply switch everything to 64bit.. > > > p->next_packet_size = 0; > > multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE); >
diff --git a/migration/multifd.c b/migration/multifd.c index 4374e14a96..498e71fd10 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque) } stat64_add(&mig_stats.multifd_bytes, - p->next_packet_size + p->packet_len); + (uint64_t)p->next_packet_size + p->packet_len); p->next_packet_size = 0; multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE);