| Message ID | 20170227203252.3295528-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Arnd, Thanks for sending this patch. On Mon, Feb 27, 2017 at 09:32:34PM +0100, Arnd Bergmann wrote: > tw5864_frameinterval_get() only initializes its output when it successfully > identifies the video standard in tw5864_input. We get a warning here because > gcc can't always track the state if initialized warnings across a WARN() > macro, and thinks it might get used incorrectly in tw5864_s_parm: > > media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm': > media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be used uninitialized in this function [-Werror=maybe-uninitialized] > media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be used uninitialized in this function [-Werror=maybe-uninitialized] I think behaviour of tw5864_frameinterval_get() is ok. I don't see how WARN() could affect gcc state tracking. There's "return -EINVAL" right after WARN() which lets caller handle the failure case gracefully. Maybe I just don't see how confusing WARN() can be for gcc in this situation, but it's not as confusing as BUG() would be, right? I see the reason of that warning is - time_base being not initialized in tw5864_s_parm() - gcc being too dumb to recognize that we have checked the retcode in tw5864_s_parm() and proceed only when we are sure we have correctly initialized time_base. Is that you compiling with manually added -Werror=maybe-uninitialized or is that default compilation flags? I don't remember encountering that and I doubt a lot of kernel code compiles without warnings with such flag. Also, which GCC version are you using? > This particular use happens to be ok, but we do copy the uninitialized > output of tw5864_frameinterval_get() into other memory without checking > the return code, interestingly without getting a warning here. Retcode checking takes place everywhere, but currently it overwrites supplied structs with potentially-uninitialized values. To make it cleaner, it should be (e.g. tw5864_g_parm()) ret = tw5864_frameinterval_get(input, &cp->timeperframe); if (ret) return ret; cp->timeperframe.numerator *= input->frame_interval; cp->capturemode = 0; cp->readbuffers = 2; return 0; and not ret = tw5864_frameinterval_get(input, &cp->timeperframe); cp->timeperframe.numerator *= input->frame_interval; cp->capturemode = 0; cp->readbuffers = 2; return ret; That would resolve your concerns of uninitialized values propagation without writing bogus values 1/1 in case of failure. I think I'd personally prefer a called function to leave my data structs intact when it fails. > > This initializes the output to 1/1s for the case, to make sure we do > get an intialization that doesn't cause a division-by-zero exception > in case we end up using this uninitialized data later. Personally I won't object against such patch, but I find it a bit too much "defensive" for kernel coding taste. Making sure somebody who doesn't check return codes don't get a crash is traditionally not considered a valid concern AFAIK. Please let me know what you think about this.
On Tue, Feb 28, 2017 at 2:08 AM, Andrey Utkin <andrey.utkin@corp.bluecherry.net> wrote: > Hi Arnd, > > Thanks for sending this patch. > > On Mon, Feb 27, 2017 at 09:32:34PM +0100, Arnd Bergmann wrote: >> tw5864_frameinterval_get() only initializes its output when it successfully >> identifies the video standard in tw5864_input. We get a warning here because >> gcc can't always track the state if initialized warnings across a WARN() >> macro, and thinks it might get used incorrectly in tw5864_s_parm: >> >> media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm': >> media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be used uninitialized in this function [-Werror=maybe-uninitialized] >> media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > I think behaviour of tw5864_frameinterval_get() is ok. > I don't see how WARN() could affect gcc state tracking. There's "return > -EINVAL" right after WARN() which lets caller handle the failure case > gracefully. Maybe I just don't see how confusing WARN() can be for gcc > in this situation, but it's not as confusing as BUG() would be, right? The problem is on architectures that use "unlikely()" within WARN(), in combination with CONFIG_PROFILE_ANNOTATED_BRANCHES(). That option invokes this macro: #define __branch_check__(x, expect) ({ \ int ______r; \ static struct ftrace_likely_data \ __attribute__((__aligned__(4))) \ __attribute__((section("_ftrace_annotated_branch"))) \ ______f = { \ .data.func = __func__, \ .data.file = __FILE__, \ .data.line = __LINE__, \ }; \ ______r = likely_notrace(x); \ ftrace_likely_update(&______f, ______r, expect); \ ______r; \ }) and after the condition has been passed through it, gcc is sufficiently confused that it forgets everything about which variables have been initialized and which haven't based on the condition. > I see the reason of that warning is > > - time_base being not initialized in tw5864_s_parm() > - gcc being too dumb to recognize that we have checked the retcode in > tw5864_s_parm() and proceed only when we are sure we have correctly > initialized time_base. > > Is that you compiling with manually added -Werror=maybe-uninitialized or > is that default compilation flags? I don't remember encountering that > and I doubt a lot of kernel code compiles without warnings with such > flag. I build with -Werror locally to turn all warnings into errors, -Wmaybe-uninitialized is turned on by default with gcc-4.9 and higher. > Also, which GCC version are you using? I see this happening on arm64 with gcc-5.2.1, gcc-6.3.1 and gcc-7.0.1, but not with gcc-4.9.3. I did not run into this one on arm or x86 with any of the above compiler versions during randconfig testing. >> This particular use happens to be ok, but we do copy the uninitialized >> output of tw5864_frameinterval_get() into other memory without checking >> the return code, interestingly without getting a warning here. > > Retcode checking takes place everywhere, but currently it overwrites > supplied structs with potentially-uninitialized values. To make it > cleaner, it should be (e.g. tw5864_g_parm()) > > ret = tw5864_frameinterval_get(input, &cp->timeperframe); > if (ret) > return ret; > cp->timeperframe.numerator *= input->frame_interval; > cp->capturemode = 0; > cp->readbuffers = 2; > return 0; > > and not > > ret = tw5864_frameinterval_get(input, &cp->timeperframe); > cp->timeperframe.numerator *= input->frame_interval; > cp->capturemode = 0; > cp->readbuffers = 2; > return ret; > > That would resolve your concerns of uninitialized values propagation > without writing bogus values 1/1 in case of failure. I think I'd > personally prefer a called function to leave my data structs intact when > it fails. That seems reasonable, I can try to come up with a new version that incorporates this change, but I haven't been able to avoid the warning without either removing the WARN() or adding an initialization. >> This initializes the output to 1/1s for the case, to make sure we do >> get an intialization that doesn't cause a division-by-zero exception >> in case we end up using this uninitialized data later. > > Personally I won't object against such patch, but I find it a bit too > much "defensive" for kernel coding taste. > > Making sure somebody who doesn't check return codes don't get a crash is > traditionally not considered a valid concern AFAIK. > > Please let me know what you think about this. I'm mainly interested in fixing the compiler warning on arm64, since I think we should have a warning. I try hard to address any related problems at the same time, and in this case it seemed justify to prepare for the other modes since the header defines additional modes that are not covered and I could not see anything preventing us from setting them: enum tw5864_vid_std { STD_NTSC = 0, /* NTSC (M) */ STD_PAL = 1, /* PAL (B, D, G, H, I) */ STD_SECAM = 2, /* SECAM */ STD_NTSC443 = 3, /* NTSC4.43 */ STD_PAL_M = 4, /* PAL (M) */ STD_PAL_CN = 5, /* PAL (CN) */ STD_PAL_60 = 6, /* PAL 60 */ STD_INVALID = 7, STD_AUTO = 7, }; Arnd
On Tue, Feb 28, 2017 at 09:20:53AM +0100, Arnd Bergmann wrote: > On Tue, Feb 28, 2017 at 2:08 AM, Andrey Utkin > <andrey.utkin@corp.bluecherry.net> wrote: > > Retcode checking takes place everywhere, but currently it overwrites > > supplied structs with potentially-uninitialized values. To make it > > cleaner, it should be (e.g. tw5864_g_parm()) > > > > ret = tw5864_frameinterval_get(input, &cp->timeperframe); > > if (ret) > > return ret; > > cp->timeperframe.numerator *= input->frame_interval; > > cp->capturemode = 0; > > cp->readbuffers = 2; > > return 0; > > > > and not > > > > ret = tw5864_frameinterval_get(input, &cp->timeperframe); > > cp->timeperframe.numerator *= input->frame_interval; > > cp->capturemode = 0; > > cp->readbuffers = 2; > > return ret; > > > > That would resolve your concerns of uninitialized values propagation > > without writing bogus values 1/1 in case of failure. I think I'd > > personally prefer a called function to leave my data structs intact when > > it fails. > > That seems reasonable, I can try to come up with a new version that > incorporates this change, but I haven't been able to avoid the warning > without either removing the WARN() or adding an initialization. I don't mind dropping WARN(). Thanks for your elaborate reply.
diff --git a/drivers/media/pci/tw5864/tw5864-video.c b/drivers/media/pci/tw5864/tw5864-video.c index 9421216bb942..a451c2081fde 100644 --- a/drivers/media/pci/tw5864/tw5864-video.c +++ b/drivers/media/pci/tw5864/tw5864-video.c @@ -728,6 +728,8 @@ static int tw5864_frameinterval_get(struct tw5864_input *input, frameinterval->denominator = 25; break; default: + frameinterval->numerator = 1; + frameinterval->denominator = 1; WARN(1, "tw5864_frameinterval_get requested for unknown std %d\n", input->std); return -EINVAL;
tw5864_frameinterval_get() only initializes its output when it successfully identifies the video standard in tw5864_input. We get a warning here because gcc can't always track the state if initialized warnings across a WARN() macro, and thinks it might get used incorrectly in tw5864_s_parm: media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm': media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be used uninitialized in this function [-Werror=maybe-uninitialized] media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be used uninitialized in this function [-Werror=maybe-uninitialized] This particular use happens to be ok, but we do copy the uninitialized output of tw5864_frameinterval_get() into other memory without checking the return code, interestingly without getting a warning here. This initializes the output to 1/1s for the case, to make sure we do get an intialization that doesn't cause a division-by-zero exception in case we end up using this uninitialized data later. This also avoids the warning. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/media/pci/tw5864/tw5864-video.c | 2 ++ 1 file changed, 2 insertions(+)