ipc: fix trivial warning
in [Kernel]
|
From: Felipe Contreras on 19 Oct 2009 12:50 On Mon, Oct 19, 2009 at 6:29 PM, Jiri Kosina <jkosina(a)suse.cz> wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: > >> >> >> ipc/msg.c: In function ?msgctl_down?: >> >> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function >> >> >> >> >> >> Signed-off-by: Felipe Contreras <felipe.contreras(a)gmail.com> >> >> >> --- >> >> >> ipc/msg.c | 2 +- >> >> >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> >> >> >> >> diff --git a/ipc/msg.c b/ipc/msg.c >> >> >> index 2ceab7f..085bd58 100644 >> >> >> --- a/ipc/msg.c >> >> >> +++ b/ipc/msg.c >> >> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd, >> >> >> struct msqid_ds __user *buf, int version) >> >> >> { >> >> >> struct kern_ipc_perm *ipcp; >> >> >> - struct msqid64_ds msqid64; >> >> >> + struct msqid64_ds uninitialized_var(msqid64); >> >> >> struct msg_queue *msq; >> >> >> int err; > [ ... snip ... ] >> > I have verified both with 4.1 and 4.3, and it doesn't emit this >> > false-positive warning, so there have been gcc versions getting this >> > right. Ergo gcc developers should rather fix this "regression" and revert >> > to the old behavior, methinks. >> >> The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4 >> finds an actual problem in the code. I will try to dig deeper to see >> what's actually happening... at first glance I don't see who is >> initializing msqid64. > > This statement of your makes me wonder why you have submitted the patch in > the first place, as you are apparently not sure whether adding > uninitialized_var() is a valid thing to do or not. Because I want to get rid of the warning? Also, if you look at the commit I mentioned in the commit message (a0d092f), you'll see this change: - struct msq_setbuf uninitialized_var(setbuf); + struct msq_setbuf setbuf; Which is ok, because in that version msgctl_down is calling audit_ipc_set_perm, directly, but only when cmd == IPC_SET. However, later on (a5f75e7), ipcctl_pre_down was introduced, and the check was delegated, and that most probably introduced the warning. In any case, if the patch is not correct, then somebody should point that out, which is what the patch review process is for. Alternatively I could have sent an email asking what is happening here, but from my point of view this patch serves exactly the same purpose, and has the advantage that it might turn out to be correct. It's not as if I didn't do any homework while writing this patch. > The gcc warning in this case is actually bogus, as msqid64 is touched only > iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes > it properly. Yes, but it's used by ipcctl_pre_down, which from what I can see is only using those arguments when cmd == IPC_SET, so everything is ok. But still, I don't think the compiler _should_ know what ipcctl_pre_down is going to do, if you look _only_ at msgctl_down, then uninitialized_var is OK. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 19 Oct 2009 19:00 On Mon, 19 Oct 2009, Felipe Contreras wrote: > > This statement of your makes me wonder why you have submitted the patch in > > the first place, as you are apparently not sure whether adding > > uninitialized_var() is a valid thing to do or not. [ ... ] > In any case, if the patch is not correct, then somebody should point > that out, which is what the patch review process is for. Alternatively > I could have sent an email asking what is happening here, but from my > point of view this patch serves exactly the same purpose, and has the > advantage that it might turn out to be correct. > > It's not as if I didn't do any homework while writing this patch. Sorry if it sounded to offensive, I have probably been a little bit too tired ysterday. > > The gcc warning in this case is actually bogus, as msqid64 is touched only > > iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes > > it properly. > > Yes, but it's used by ipcctl_pre_down, which from what I can see is > only using those arguments when cmd == IPC_SET, so everything is ok. > But still, I don't think the compiler _should_ know what > ipcctl_pre_down is going to do, if you look _only_ at msgctl_down, > then uninitialized_var is OK. Well at least older compilers were able to see this, only 4.4 seems to be warning here ... I have applied the patch for now, but I am really not fully convinced about it yet, we should probably report it to gcc folks anyway. Thanks, -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Felipe Contreras on 11 Nov 2009 19:30 On Tue, Oct 20, 2009 at 12:50 AM, Jiri Kosina <jkosina(a)suse.cz> wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: >> Yes, but it's used by ipcctl_pre_down, which from what I can see is >> only using those arguments when cmd == IPC_SET, so everything is ok. >> But still, I don't think the compiler _should_ know what >> ipcctl_pre_down is going to do, if you look _only_ at msgctl_down, >> then uninitialized_var is OK. > > Well at least older compilers were able to see this, only 4.4 seems to be > warning here ... I have applied the patch for now, but I am really not > fully convinced about it yet, we should probably report it to gcc folks > anyway. I have in my to-do list to report this to the gcc guys, but in the meantime I don't see this patch applied. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 11 Nov 2009 20:20 On Thu, 12 Nov 2009, Felipe Contreras wrote: > > Well at least older compilers were able to see this, only 4.4 seems to be > > warning here ... I have applied the patch for now, but I am really not > > fully convinced about it yet, we should probably report it to gcc folks > > anyway. > > I have in my to-do list to report this to the gcc guys, but in the > meantime I don't see this patch applied. It is applied in my tree, scheduled for next merge window. -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
First
|
Prev
|
Pages: 1 2 Prev: drivers: Remove BKL from misc_open Next: nfs oops in 2.6.32-rc5-00041-g1d91624 |