| Message ID | alpine.DEB.2.21.1906191422170.2072@sstabellini-ThinkPad-T480s (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: fix build after 2e35cdf | expand |
Hi Stefano, Title: You should at least mention this is for op-tee. Also, mostly likely the sha1 is too small and likely to match multiple commit in the future. So you want to specify the title of the commit. On 6/19/19 10:24 PM, Stefano Stabellini wrote: > Optee breaks the build with: > > optee.c: In function ‘translate_noncontig.isra.4’: > optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > xen_data->next_page_data = page_to_maddr(xen_pgs + 1); > ^ > optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); > ^ > optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > put_page(guest_pg); > ^ > cc1: all warnings being treated as errors > > Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set > xen_pgs to NULL for consistency. Without more explanation I think this is an unwise choice. If GCC thinks it is going to be used unitialized, then mostly likely you silent an error that could end up to dereference NULL. Also, setting xen_pgs for consistency will only defeat the compiler. Leading to dereferencing NULL and crash Xen... For xen_pgs, this should definitely not be NULL. For the two others, you need to explain why this is fine (if this is just because the compiler can't find the reason, then you should add a comment in the code to explain it). > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c > index 28d34360fc..4825cc5410 100644 > --- a/xen/arch/arm/tee/optee.c > +++ b/xen/arch/arm/tee/optee.c > @@ -663,7 +663,7 @@ static int translate_noncontig(struct optee_domain *ctx, > unsigned int order; > unsigned int idx = 0; > gfn_t gfn; > - struct page_info *guest_pg, *xen_pgs; > + struct page_info *guest_pg = NULL, *xen_pgs = NULL; > struct optee_shm_buf *optee_shm_buf; > /* > * This is memory layout for page list. Basically list consists of 4k pages, > @@ -675,7 +675,7 @@ static int translate_noncontig(struct optee_domain *ctx, > struct { > uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE]; > uint64_t next_page_data; > - } *guest_data, *xen_data; > + } *guest_data = NULL, *xen_data = NULL; > > /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */ > offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1); >
On Wed, 19 Jun 2019, Julien Grall wrote: > Hi Stefano, > > Title: You should at least mention this is for op-tee. > > Also, mostly likely the sha1 is too small and likely to match multiple commit > in the future. So you want to specify the title of the commit. > > On 6/19/19 10:24 PM, Stefano Stabellini wrote: > > Optee breaks the build with: > > > > optee.c: In function ‘translate_noncontig.isra.4’: > > optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > xen_data->next_page_data = page_to_maddr(xen_pgs + 1); > > ^ > > optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this > > function [-Werror=maybe-uninitialized] > > page = > > get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); > > ^ > > optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > put_page(guest_pg); > > ^ > > cc1: all warnings being treated as errors > > > > Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set > > xen_pgs to NULL for consistency. > > Without more explanation I think this is an unwise choice. If GCC thinks it is > going to be used unitialized, then mostly likely you silent an error that > could end up to dereference NULL. > > Also, setting xen_pgs for consistency will only defeat the compiler. Leading > to dereferencing NULL and crash Xen... > > For xen_pgs, this should definitely not be NULL. For the two others, you need > to explain why this is fine (if this is just because the compiler can't find > the reason, then you should add a comment in the code to explain it). I was only trying to unblock the build. I'll withdraw the patch and let Volodmir fix it properly. Volodmir, FYI I reproduced the issue using Ubuntu Trusty gcc 4.8.4-2ubuntu1~14.04.3.
On 6/19/19 10:47 PM, Stefano Stabellini wrote: > On Wed, 19 Jun 2019, Julien Grall wrote: >> Hi Stefano, >> >> Title: You should at least mention this is for op-tee. >> >> Also, mostly likely the sha1 is too small and likely to match multiple commit >> in the future. So you want to specify the title of the commit. >> >> On 6/19/19 10:24 PM, Stefano Stabellini wrote: >>> Optee breaks the build with: >>> >>> optee.c: In function ‘translate_noncontig.isra.4’: >>> optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function >>> [-Werror=maybe-uninitialized] >>> xen_data->next_page_data = page_to_maddr(xen_pgs + 1); >>> ^ >>> optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this >>> function [-Werror=maybe-uninitialized] >>> page = >>> get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); >>> ^ >>> optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function >>> [-Werror=maybe-uninitialized] >>> put_page(guest_pg); >>> ^ >>> cc1: all warnings being treated as errors >>> >>> Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set >>> xen_pgs to NULL for consistency. >> >> Without more explanation I think this is an unwise choice. If GCC thinks it is >> going to be used unitialized, then mostly likely you silent an error that >> could end up to dereference NULL. >> >> Also, setting xen_pgs for consistency will only defeat the compiler. Leading >> to dereferencing NULL and crash Xen... >> >> For xen_pgs, this should definitely not be NULL. For the two others, you need >> to explain why this is fine (if this is just because the compiler can't find >> the reason, then you should add a comment in the code to explain it). > > I was only trying to unblock the build. So? We don't silence a compiler warning just for unblocking the build without any proper investigation. Didn't you do that before adding the NULL? Cheers,
On Wed, 19 Jun 2019, Julien Grall wrote: > On 6/19/19 10:47 PM, Stefano Stabellini wrote: > > On Wed, 19 Jun 2019, Julien Grall wrote: > > > Hi Stefano, > > > > > > Title: You should at least mention this is for op-tee. > > > > > > Also, mostly likely the sha1 is too small and likely to match multiple > > > commit > > > in the future. So you want to specify the title of the commit. > > > > > > On 6/19/19 10:24 PM, Stefano Stabellini wrote: > > > > Optee breaks the build with: > > > > > > > > optee.c: In function ‘translate_noncontig.isra.4’: > > > > optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this > > > > function > > > > [-Werror=maybe-uninitialized] > > > > xen_data->next_page_data = page_to_maddr(xen_pgs + 1); > > > > ^ > > > > optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this > > > > function [-Werror=maybe-uninitialized] > > > > page = > > > > get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); > > > > ^ > > > > optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this > > > > function > > > > [-Werror=maybe-uninitialized] > > > > put_page(guest_pg); > > > > ^ > > > > cc1: all warnings being treated as errors > > > > > > > > Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set > > > > xen_pgs to NULL for consistency. > > > > > > Without more explanation I think this is an unwise choice. If GCC thinks > > > it is > > > going to be used unitialized, then mostly likely you silent an error that > > > could end up to dereference NULL. > > > > > > Also, setting xen_pgs for consistency will only defeat the compiler. > > > Leading > > > to dereferencing NULL and crash Xen... > > > > > > For xen_pgs, this should definitely not be NULL. For the two others, you > > > need > > > to explain why this is fine (if this is just because the compiler can't > > > find > > > the reason, then you should add a comment in the code to explain it). > > > > I was only trying to unblock the build. > > So? We don't silence a compiler warning just for unblocking the build without > any proper investigation. Didn't you do that before adding the NULL? No I didn't. But actually, I thought we did unbreak a build as quickly as possible even without a full fix in the past. In fact, I seem to recollect that we did that even without collecting all necessary acks. Maybe my memory is failing me? But I would have sworn it happened a couple of times in the last 12 months. Or maybe this case is different because it doesn't break the build with the default kconfig? In any case, let's agree on a policy and I am happy to follow it.
Hi Stefano, On 6/19/19 11:04 PM, Stefano Stabellini wrote: > On Wed, 19 Jun 2019, Julien Grall wrote: >> On 6/19/19 10:47 PM, Stefano Stabellini wrote: >>> On Wed, 19 Jun 2019, Julien Grall wrote: >>>> Hi Stefano, >>>> >>>> Title: You should at least mention this is for op-tee. >>>> >>>> Also, mostly likely the sha1 is too small and likely to match multiple >>>> commit >>>> in the future. So you want to specify the title of the commit. >>>> >>>> On 6/19/19 10:24 PM, Stefano Stabellini wrote: >>>>> Optee breaks the build with: >>>>> >>>>> optee.c: In function ‘translate_noncontig.isra.4’: >>>>> optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this >>>>> function >>>>> [-Werror=maybe-uninitialized] >>>>> xen_data->next_page_data = page_to_maddr(xen_pgs + 1); >>>>> ^ >>>>> optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this >>>>> function [-Werror=maybe-uninitialized] >>>>> page = >>>>> get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); >>>>> ^ >>>>> optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this >>>>> function >>>>> [-Werror=maybe-uninitialized] >>>>> put_page(guest_pg); >>>>> ^ >>>>> cc1: all warnings being treated as errors >>>>> >>>>> Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set >>>>> xen_pgs to NULL for consistency. >>>> >>>> Without more explanation I think this is an unwise choice. If GCC thinks >>>> it is >>>> going to be used unitialized, then mostly likely you silent an error that >>>> could end up to dereference NULL. >>>> >>>> Also, setting xen_pgs for consistency will only defeat the compiler. >>>> Leading >>>> to dereferencing NULL and crash Xen... >>>> >>>> For xen_pgs, this should definitely not be NULL. For the two others, you >>>> need >>>> to explain why this is fine (if this is just because the compiler can't >>>> find >>>> the reason, then you should add a comment in the code to explain it). >>> >>> I was only trying to unblock the build. >> >> So? We don't silence a compiler warning just for unblocking the build without >> any proper investigation. Didn't you do that before adding the NULL? > > No I didn't. But actually, I thought we did unbreak a build as quickly > as possible even without a full fix in the past. And who is going to do the follow-up? AFAICT, you will not be the one and therefore that's a call for this to stay as it is in Xen. > In fact, I seem to > recollect that we did that even without collecting all necessary acks. Collecting the necessary acks and not investigating are something totally different. There are a couple of instance where patch went without the necessary acks to unblock build/test (see Jan's series for 4.10 and 4.11). However Jan still investigated the problem. > Maybe my memory is failing me? But I would have sworn it happened a > couple of times in the last 12 months. Or maybe this case is different > because it doesn't break the build with the default kconfig? In any > case, let's agree on a policy and I am happy to follow it. This can't be reached with osstest (as it is protected by EXPERT), but I didn't base my judgment on that. I based my judgment on the compiler reporting a potential error and the commit message not explaining why setting to NULL would be ok. I am happy to have build fix going without any acks (to certain extend), however we should not lower down the quality of the commit for that. Cheers,
Hi Stefano, Julien, Stefano Stabellini writes: > On Wed, 19 Jun 2019, Julien Grall wrote: >> Hi Stefano, >> >> Title: You should at least mention this is for op-tee. >> >> Also, mostly likely the sha1 is too small and likely to match multiple commit >> in the future. So you want to specify the title of the commit. >> >> On 6/19/19 10:24 PM, Stefano Stabellini wrote: >> > Optee breaks the build with: >> > >> > optee.c: In function ‘translate_noncontig.isra.4’: >> > optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function >> > [-Werror=maybe-uninitialized] >> > xen_data->next_page_data = page_to_maddr(xen_pgs + 1); >> > ^ >> > optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this >> > function [-Werror=maybe-uninitialized] >> > page = >> > get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); >> > ^ >> > optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function >> > [-Werror=maybe-uninitialized] >> > put_page(guest_pg); >> > ^ >> > cc1: all warnings being treated as errors >> > >> > Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set >> > xen_pgs to NULL for consistency. >> >> Without more explanation I think this is an unwise choice. If GCC thinks it is >> going to be used unitialized, then mostly likely you silent an error that >> could end up to dereference NULL. There is no way to use this variables without initialization. They are always initialized on the first iteration of the loop, when idx equals to 0. Newer version of GCC can infer this, but look like this causes problem for older versions. >> Also, setting xen_pgs for consistency will only defeat the compiler. Leading >> to dereferencing NULL and crash Xen... >> >> For xen_pgs, this should definitely not be NULL. For the two others, you need >> to explain why this is fine (if this is just because the compiler can't find >> the reason, then you should add a comment in the code to explain it). > > I was only trying to unblock the build. I'll withdraw the patch and let > Volodmir fix it properly. Actually, your patch is fine, taking into account Julien's comment about xen_pgs and justification in the comments. So, you can send fixed version or I can do this, if you don't want to. > > Volodmir, FYI I reproduced the issue using Ubuntu Trusty gcc > 4.8.4-2ubuntu1~14.04.3. Oh, I see. This is the quite old version of GCC. I'm using aarch64-poky-linux-gcc (Linaro GCC 5.2-2015.11-2) 5.2.1 20151005 which is also old, but at least it tracks variables initialization better. -- Best regards,Volodymyr Babchuk
On 20/06/2019 12:05, Volodymyr Babchuk wrote: >> Volodmir, FYI I reproduced the issue using Ubuntu Trusty gcc >> 4.8.4-2ubuntu1~14.04.3. > Oh, I see. This is the quite old version of GCC. I'm using > aarch64-poky-linux-gcc (Linaro GCC 5.2-2015.11-2) 5.2.1 20151005 > which is also old, but at least it tracks variables initialization > better. This was spotted by CI https://travis-ci.org/andyhhp/xen/jobs/547896353 and is a supported of GCC which Xen supports. ~Andrew
>>> On 20.06.19 at 12:00, <julien.grall@arm.com> wrote: > Hi Stefano, > > On 6/19/19 11:04 PM, Stefano Stabellini wrote: >> On Wed, 19 Jun 2019, Julien Grall wrote: >>> On 6/19/19 10:47 PM, Stefano Stabellini wrote: >>>> On Wed, 19 Jun 2019, Julien Grall wrote: >>>>> Hi Stefano, >>>>> >>>>> Title: You should at least mention this is for op-tee. >>>>> >>>>> Also, mostly likely the sha1 is too small and likely to match multiple >>>>> commit >>>>> in the future. So you want to specify the title of the commit. >>>>> >>>>> On 6/19/19 10:24 PM, Stefano Stabellini wrote: >>>>>> Optee breaks the build with: >>>>>> >>>>>> optee.c: In function ‘translate_noncontig.isra.4’: >>>>>> optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this >>>>>> function >>>>>> [-Werror=maybe-uninitialized] >>>>>> xen_data->next_page_data = page_to_maddr(xen_pgs + 1); >>>>>> ^ >>>>>> optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this >>>>>> function [-Werror=maybe-uninitialized] >>>>>> page = >>>>>> get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); >>>>>> ^ >>>>>> optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this >>>>>> function >>>>>> [-Werror=maybe-uninitialized] >>>>>> put_page(guest_pg); >>>>>> ^ >>>>>> cc1: all warnings being treated as errors >>>>>> >>>>>> Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set >>>>>> xen_pgs to NULL for consistency. >>>>> >>>>> Without more explanation I think this is an unwise choice. If GCC thinks >>>>> it is >>>>> going to be used unitialized, then mostly likely you silent an error that >>>>> could end up to dereference NULL. >>>>> >>>>> Also, setting xen_pgs for consistency will only defeat the compiler. >>>>> Leading >>>>> to dereferencing NULL and crash Xen... >>>>> >>>>> For xen_pgs, this should definitely not be NULL. For the two others, you >>>>> need >>>>> to explain why this is fine (if this is just because the compiler can't >>>>> find >>>>> the reason, then you should add a comment in the code to explain it). >>>> >>>> I was only trying to unblock the build. >>> >>> So? We don't silence a compiler warning just for unblocking the build > without >>> any proper investigation. Didn't you do that before adding the NULL? >> >> No I didn't. But actually, I thought we did unbreak a build as quickly >> as possible even without a full fix in the past. > > And who is going to do the follow-up? AFAICT, you will not be the one > and therefore that's a call for this to stay as it is in Xen. > >> In fact, I seem to >> recollect that we did that even without collecting all necessary acks. > > Collecting the necessary acks and not investigating are something > totally different. There are a couple of instance where patch went > without the necessary acks to unblock build/test (see Jan's series for > 4.10 and 4.11). > > However Jan still investigated the problem. > >> Maybe my memory is failing me? But I would have sworn it happened a >> couple of times in the last 12 months. Or maybe this case is different >> because it doesn't break the build with the default kconfig? In any >> case, let's agree on a policy and I am happy to follow it. > > This can't be reached with osstest (as it is protected by EXPERT), but I > didn't base my judgment on that. > > I based my judgment on the compiler reporting a potential error and the > commit message not explaining why setting to NULL would be ok. > > I am happy to have build fix going without any acks (to certain extend), > however we should not lower down the quality of the commit for that. Since Julien asked for an explicit opinion: I fully agree with him here. Yes, we have been rushing in build fixes, but only when there was basically no doubt that everyone who would normally have to ack such a change wouldn't object. This in particular implies not just papering over issues. Jan
diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c index 28d34360fc..4825cc5410 100644 --- a/xen/arch/arm/tee/optee.c +++ b/xen/arch/arm/tee/optee.c @@ -663,7 +663,7 @@ static int translate_noncontig(struct optee_domain *ctx, unsigned int order; unsigned int idx = 0; gfn_t gfn; - struct page_info *guest_pg, *xen_pgs; + struct page_info *guest_pg = NULL, *xen_pgs = NULL; struct optee_shm_buf *optee_shm_buf; /* * This is memory layout for page list. Basically list consists of 4k pages, @@ -675,7 +675,7 @@ static int translate_noncontig(struct optee_domain *ctx, struct { uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE]; uint64_t next_page_data; - } *guest_data, *xen_data; + } *guest_data = NULL, *xen_data = NULL; /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */ offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
Optee breaks the build with: optee.c: In function ‘translate_noncontig.isra.4’: optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function [-Werror=maybe-uninitialized] xen_data->next_page_data = page_to_maddr(xen_pgs + 1); ^ optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this function [-Werror=maybe-uninitialized] page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx])); ^ optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function [-Werror=maybe-uninitialized] put_page(guest_pg); ^ cc1: all warnings being treated as errors Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set xen_pgs to NULL for consistency. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>