Re: [RFC] kvm - possible out of bounds
2015-11-29 18:33 GMT-03:00 Paul Mackerras :
> On Sun, Nov 29, 2015 at 05:14:03PM -0300, Geyslan Gregório Bem wrote:
>> Hello,
>>
>> I have found a possible out of bounds reading in
>> arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
>> function). pteg[] array could be accessed twice using the i variable
>> after the for iteration. What happens is that in the last iteration
>> the i index is incremented to 16, checked (i<16) then confirmed
>> exiting the loop.
>>
>> 277for (i=0; i<16; i+=2) { ...
>>
>> Later there are reading attempts to the pteg last elements, but using
>> again the already incremented i (16).
>>
>> 303v = be64_to_cpu(pteg[i]); /* pteg[16] */
>> 304r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
>
> Was it some automated tool that came up with this?
Yep, cppcheck. As I'm still not engaged to a specific area in the
kernel, just trying to help with automated catches.
>
> There is actually no problem because the accesses outside the loop are
> only done if the 'found' variable is true; 'found' is initialized to
> false and only ever set to true inside the loop just before a break
> statement. Thus there is a correlation between the value of 'i' and
> the value of 'found' -- if 'found' is true then we know 'i' is less
> than 16.
I figured it out after your explanation.
>
>> I really don't know if the for lace will somehow iterate until i is
>> 16, anyway I think that the last readings must be using a defined max
>> len/index or another more clear method.
>
> I think it's perfectly clear to a human programmer, though some tools
> (such as gcc) struggle with this kind of correlation between
> variables. That's why I asked whether your report was based on the
> output from some tool.
>
>> Eg.
>>
>> v = be64_to_cpu(pteg[PTEG_LEN - 2]);
>> r = be64_to_cpu(pteg[PTEG_LEN - 1]);
>>
>> Or just.
>>
>> v = be64_to_cpu(pteg[14]);
>> r = be64_to_cpu(pteg[15]);
>
> Either of those options would cause the code to malfunction.
Yep, I understand now that v and r get the found ones. So i is needed.
>
>> I found in the same file a variable that is not used.
>>
>> 380struct kvmppc_vcpu_book3s *vcpu_book3s;
>> ...
>> 387vcpu_book3s = to_book3s(vcpu);
>
> True. It could be removed.
I'll make a patch for that.
>
>> A question, the kvmppc_mmu_book3s_64_init function is accessed by
>> unconventional way? Because I have not found any calling to it.
>
> Try arch/powerpc/kvm/book3s_pr.c line 410:
>
> kvmppc_mmu_book3s_64_init(vcpu);
>
> Grep (or git grep) is your friend.
Hmm, indeed.
>
> Paul.
Thank you, Paul. If you have some other changes in progress let me know.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] kvm - possible out of bounds
Hello,
I have found a possible out of bounds reading in
arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
function). pteg[] array could be accessed twice using the i variable
after the for iteration. What happens is that in the last iteration
the i index is incremented to 16, checked (i<16) then confirmed
exiting the loop.
277for (i=0; i<16; i+=2) { ...
Later there are reading attempts to the pteg last elements, but using
again the already incremented i (16).
303v = be64_to_cpu(pteg[i]); /* pteg[16] */
304r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
I really don't know if the for lace will somehow iterate until i is
16, anyway I think that the last readings must be using a defined max
len/index or another more clear method.
Eg.
v = be64_to_cpu(pteg[PTEG_LEN - 2]);
r = be64_to_cpu(pteg[PTEG_LEN - 1]);
Or just.
v = be64_to_cpu(pteg[14]);
r = be64_to_cpu(pteg[15]);
I found in the same file a variable that is not used.
380struct kvmppc_vcpu_book3s *vcpu_book3s;
...
387vcpu_book3s = to_book3s(vcpu);
-
A question, the kvmppc_mmu_book3s_64_init function is accessed by
unconventional way? Because I have not found any calling to it.
If something that I wrote is correct please tell me if I could send the patch.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] arch/ia64/kernel/palinfo.c: bitvector_process reading out of bounds
Hello,
I'm doing some static analysis and stumbled in this function
static void bitvector_process(struct seq_file *m, u64 vector)
{
int i,j;
static const char *units[]={ "", "K", "M", "G", "T" };
for (i=0, j=0; i < 64; i++ , j=i/10) {
if (vector & 0x1)
seq_printf(m, "%d%s ", 1 << (i-j*10), units[j]);
vector >>= 1;
}
}
It appears that units[] (5 elements) can be accessed out of bounds in
seq_printf call
seq_printf(m, "%d%s ", 1 << (i-j*10), units[j]);
once j is being set to i/10.
i goes from 0 to 63 (u64 bits length), and when vector & 1 (odd),
units[j] will calculate outside the boundaries when vector get close
to Petabyte magnitude.
Well, as bitvector_process doesn't control the max size of vector and
the future is knocking on door, I would suggest this change
-static const char *units[]={ "", "K", "M", "G", "T" };
+static const char *units[]={ "", "K", "M", "G", "T", "P", "E" };
then if the u64 max value (18446744073709551615) is used the array
will provide the correct (E) suffix.
If that change is not pertinent I would like to know why.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] kvm - possible out of bounds
Hello,
I have found a possible out of bounds reading in
arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
function). pteg[] array could be accessed twice using the i variable
after the for iteration. What happens is that in the last iteration
the i index is incremented to 16, checked (i<16) then confirmed
exiting the loop.
277for (i=0; i<16; i+=2) { ...
Later there are reading attempts to the pteg last elements, but using
again the already incremented i (16).
303v = be64_to_cpu(pteg[i]); /* pteg[16] */
304r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
I really don't know if the for lace will somehow iterate until i is
16, anyway I think that the last readings must be using a defined max
len/index or another more clear method.
Eg.
v = be64_to_cpu(pteg[PTEG_LEN - 2]);
r = be64_to_cpu(pteg[PTEG_LEN - 1]);
Or just.
v = be64_to_cpu(pteg[14]);
r = be64_to_cpu(pteg[15]);
I found in the same file a variable that is not used.
380struct kvmppc_vcpu_book3s *vcpu_book3s;
...
387vcpu_book3s = to_book3s(vcpu);
-
A question, the kvmppc_mmu_book3s_64_init function is accessed by
unconventional way? Because I have not found any calling to it.
If something that I wrote is correct please tell me if I could send the patch.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] kvm - possible out of bounds
2015-11-29 18:33 GMT-03:00 Paul Mackerras <pau...@ozlabs.org>:
> On Sun, Nov 29, 2015 at 05:14:03PM -0300, Geyslan Gregório Bem wrote:
>> Hello,
>>
>> I have found a possible out of bounds reading in
>> arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
>> function). pteg[] array could be accessed twice using the i variable
>> after the for iteration. What happens is that in the last iteration
>> the i index is incremented to 16, checked (i<16) then confirmed
>> exiting the loop.
>>
>> 277for (i=0; i<16; i+=2) { ...
>>
>> Later there are reading attempts to the pteg last elements, but using
>> again the already incremented i (16).
>>
>> 303v = be64_to_cpu(pteg[i]); /* pteg[16] */
>> 304r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
>
> Was it some automated tool that came up with this?
Yep, cppcheck. As I'm still not engaged to a specific area in the
kernel, just trying to help with automated catches.
>
> There is actually no problem because the accesses outside the loop are
> only done if the 'found' variable is true; 'found' is initialized to
> false and only ever set to true inside the loop just before a break
> statement. Thus there is a correlation between the value of 'i' and
> the value of 'found' -- if 'found' is true then we know 'i' is less
> than 16.
I figured it out after your explanation.
>
>> I really don't know if the for lace will somehow iterate until i is
>> 16, anyway I think that the last readings must be using a defined max
>> len/index or another more clear method.
>
> I think it's perfectly clear to a human programmer, though some tools
> (such as gcc) struggle with this kind of correlation between
> variables. That's why I asked whether your report was based on the
> output from some tool.
>
>> Eg.
>>
>> v = be64_to_cpu(pteg[PTEG_LEN - 2]);
>> r = be64_to_cpu(pteg[PTEG_LEN - 1]);
>>
>> Or just.
>>
>> v = be64_to_cpu(pteg[14]);
>> r = be64_to_cpu(pteg[15]);
>
> Either of those options would cause the code to malfunction.
Yep, I understand now that v and r get the found ones. So i is needed.
>
>> I found in the same file a variable that is not used.
>>
>> 380struct kvmppc_vcpu_book3s *vcpu_book3s;
>> ...
>> 387vcpu_book3s = to_book3s(vcpu);
>
> True. It could be removed.
I'll make a patch for that.
>
>> A question, the kvmppc_mmu_book3s_64_init function is accessed by
>> unconventional way? Because I have not found any calling to it.
>
> Try arch/powerpc/kvm/book3s_pr.c line 410:
>
> kvmppc_mmu_book3s_64_init(vcpu);
>
> Grep (or git grep) is your friend.
Hmm, indeed.
>
> Paul.
Thank you, Paul. If you have some other changes in progress let me know.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] arch/ia64/kernel/palinfo.c: bitvector_process reading out of bounds
Hello,
I'm doing some static analysis and stumbled in this function
static void bitvector_process(struct seq_file *m, u64 vector)
{
int i,j;
static const char *units[]={ "", "K", "M", "G", "T" };
for (i=0, j=0; i < 64; i++ , j=i/10) {
if (vector & 0x1)
seq_printf(m, "%d%s ", 1 << (i-j*10), units[j]);
vector >>= 1;
}
}
It appears that units[] (5 elements) can be accessed out of bounds in
seq_printf call
seq_printf(m, "%d%s ", 1 << (i-j*10), units[j]);
once j is being set to i/10.
i goes from 0 to 63 (u64 bits length), and when vector & 1 (odd),
units[j] will calculate outside the boundaries when vector get close
to Petabyte magnitude.
Well, as bitvector_process doesn't control the max size of vector and
the future is knocking on door, I would suggest this change
-static const char *units[]={ "", "K", "M", "G", "T" };
+static const char *units[]={ "", "K", "M", "G", "T", "P", "E" };
then if the u64 max value (18446744073709551615) is used the array
will provide the correct (E) suffix.
If that change is not pertinent I would like to know why.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Kernel-BR] Re: [RFC] Only a.out QMAGIC format is working
Valdis, 2014-04-19 15:33 GMT-03:00 : > On Sat, 19 Apr 2014 13:37:27 -0300, Geyslan Gregório Bem said: > >> Maintainers, is there some chance to fix it or a.out is really doomed? > > Is there an actual use case for a.out on a modern kernel? Maybe retrocompatibility. > > In other wods, is there any reason to really care if it's doomed, since > it's been *years* since that worked? Perhaps not, but why to continue patching the ia32_aout.c and binfmt_aout.c if that format doesn't matter (and work) anymore? That's was the reason that I asked if it is really doomed in linux. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Kernel-BR] Re: [RFC] Only a.out QMAGIC format is working
Pavel, Thank you. Maintainers, is there some chance to fix it or a.out is really doomed? 2014-04-19 13:15 GMT-03:00 Pavel Machek : > Hi! > >> I was researching about old binary formats and did some tests. >> Meantime, I was able to run sucessfully only the QMAGIC format. >> Nonetheless, the OMAGIC, NMAGIC and ZMAGIC didn't work anymore. > > Some time ago, I ran into similar problem, and turning off userspace > randomization was neccessary... Jiri Kosina might remember details. > > Pavel > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > -- > Você está recebendo esta mensagem porque se inscreveu no grupo "Kernel > Brasil" dos Grupos do Google. > Para cancelar inscrição nesse grupo e parar de receber e-mails dele, envie um > e-mail para kernel-br+unsubscr...@googlegroups.com. > Para postar neste grupo, envie um e-mail para kernel...@googlegroups.com. > Para ver esta discussão na web, acesse > https://groups.google.com/d/msgid/kernel-br/20140419161552.GB27776%40amd.pavel.ucw.cz. > Para obter mais opções, acesse https://groups.google.com/d/optout. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Kernel-BR] Re: [RFC] Only a.out QMAGIC format is working
Pavel, Thank you. Maintainers, is there some chance to fix it or a.out is really doomed? 2014-04-19 13:15 GMT-03:00 Pavel Machek pa...@ucw.cz: Hi! I was researching about old binary formats and did some tests. Meantime, I was able to run sucessfully only the QMAGIC format. Nonetheless, the OMAGIC, NMAGIC and ZMAGIC didn't work anymore. Some time ago, I ran into similar problem, and turning off userspace randomization was neccessary... Jiri Kosina might remember details. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- Você está recebendo esta mensagem porque se inscreveu no grupo Kernel Brasil dos Grupos do Google. Para cancelar inscrição nesse grupo e parar de receber e-mails dele, envie um e-mail para kernel-br+unsubscr...@googlegroups.com. Para postar neste grupo, envie um e-mail para kernel...@googlegroups.com. Para ver esta discussão na web, acesse https://groups.google.com/d/msgid/kernel-br/20140419161552.GB27776%40amd.pavel.ucw.cz. Para obter mais opções, acesse https://groups.google.com/d/optout. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Kernel-BR] Re: [RFC] Only a.out QMAGIC format is working
Valdis, 2014-04-19 15:33 GMT-03:00 valdis.kletni...@vt.edu: On Sat, 19 Apr 2014 13:37:27 -0300, Geyslan Gregório Bem said: Maintainers, is there some chance to fix it or a.out is really doomed? Is there an actual use case for a.out on a modern kernel? Maybe retrocompatibility. In other wods, is there any reason to really care if it's doomed, since it's been *years* since that worked? Perhaps not, but why to continue patching the ia32_aout.c and binfmt_aout.c if that format doesn't matter (and work) anymore? That's was the reason that I asked if it is really doomed in linux. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] Only a.out QMAGIC format is working
Sirs,
I was researching about old binary formats and did some tests.
Meantime, I was able to run sucessfully only the QMAGIC format.
Nonetheless, the OMAGIC, NMAGIC and ZMAGIC didn't work anymore.
The test occurred using old slackware binaries and some new, the
latter compiled by me, using cross-compiled as and ld. In any case,
the QMAGIC was the only functional.
After some debugging I identified (when loading a OMAGIC) that the
kernel sigkill the current after this checking:
http://lxr.linux.no/linux+v3.13.5/arch/x86/ia32/ia32_aout.c#L325
325error = vm_brk(text_addr & PAGE_MASK, map_size);
326
327if (error != (text_addr & PAGE_MASK)) {
328send_sig(SIGKILL, current, 0);
329return error;
330}
I suppose this happens due to changes made, in the course of time, in
the memory mapping (vm_brk/do_brk), therefore the only one that still
works is the QMAGIC (the aligned one). Or maybe, it's purposely. [RFC]
Is important to note that when a ZMAGIC is loaded what happens is a
"Segmentation fault" and not "SIGKILL".
That was reported by others too:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/966472
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] Only a.out QMAGIC format is working
Sirs,
I was researching about old binary formats and did some tests.
Meantime, I was able to run sucessfully only the QMAGIC format.
Nonetheless, the OMAGIC, NMAGIC and ZMAGIC didn't work anymore.
The test occurred using old slackware binaries and some new, the
latter compiled by me, using cross-compiled as and ld. In any case,
the QMAGIC was the only functional.
After some debugging I identified (when loading a OMAGIC) that the
kernel sigkill the current after this checking:
http://lxr.linux.no/linux+v3.13.5/arch/x86/ia32/ia32_aout.c#L325
325error = vm_brk(text_addr PAGE_MASK, map_size);
326
327if (error != (text_addr PAGE_MASK)) {
328send_sig(SIGKILL, current, 0);
329return error;
330}
I suppose this happens due to changes made, in the course of time, in
the memory mapping (vm_brk/do_brk), therefore the only one that still
works is the QMAGIC (the aligned one). Or maybe, it's purposely. [RFC]
Is important to note that when a ZMAGIC is loaded what happens is a
Segmentation fault and not SIGKILL.
That was reported by others too:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/966472
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] double free in decompressor.c
2013/11/22 Phillip Lougher :
> On 22/11/13 21:50, Geyslan Gregório Bem wrote:
>>
>> Coverity caught double free possibility (CID 1130962).
>>
>> I can patch this, but I have to know if is correct to free comp_opts
>> in the function squashfs_decompressor_create() or it had to be done in
>> the caller. My bet is the caller.
>>
>>
>> 128void *squashfs_decompressor_setup(struct super_block *sb, unsigned
>> short flags)
>> 129{
>> 130struct squashfs_sb_info *msblk = sb->s_fs_info;
>> 131void *stream, *comp_opts = get_comp_opts(sb, flags);
>> 132
>>
>> 1. Condition "IS_ERR(comp_opts)", taking false branch
>> 133if (IS_ERR(comp_opts))
>> 134return comp_opts;
>> 135
>>
>> 2. freed_arg: "squashfs_decompressor_create(struct squashfs_sb_info *,
>> void *)" frees "comp_opts".[show details]
>> 136stream = squashfs_decompressor_create(msblk, comp_opts);
>>
>> 3. Condition "IS_ERR(stream)", taking true branch
>> 137if (IS_ERR(stream))
>
>
> FALSE positive.
>
> squashfs_decompressor_create() frees comp_opts only on success.
>
> If IS_ERR(stream) is true, then comp_opts has not been freed by
> squashfs_decompressor_create().
>
> Phillip
>
>
>
>>
>> CID 1130962 (#1 of 1): Double free (USE_AFTER_FREE)4. double_free:
>> Calling "kfree(void const *)" frees pointer "comp_opts" which has
>> already been freed.
>> 138kfree(comp_opts);
>> 139
>> 140return stream;
>> 141}
>>
>>
>
Philip, set as false positive in Coverity. Thanks.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] double free in decompressor.c
2013/11/22 Richard Weinberger : > On Fri, Nov 22, 2013 at 10:50 PM, Geyslan Gregório Bem > wrote: >> Coverity caught double free possibility (CID 1130962). > > Just wondering, where can one find/verify such CIDs? > > -- > Thanks, > //richard Anyone can sign in (https://scan.coverity.com/) and choose an open source project or register one (as maintainer) indeed. After that, configure to receive "new defects" updates. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] double free in decompressor.c
Coverity caught double free possibility (CID 1130962).
I can patch this, but I have to know if is correct to free comp_opts
in the function squashfs_decompressor_create() or it had to be done in
the caller. My bet is the caller.
128void *squashfs_decompressor_setup(struct super_block *sb, unsigned
short flags)
129{
130struct squashfs_sb_info *msblk = sb->s_fs_info;
131void *stream, *comp_opts = get_comp_opts(sb, flags);
132
1. Condition "IS_ERR(comp_opts)", taking false branch
133if (IS_ERR(comp_opts))
134return comp_opts;
135
2. freed_arg: "squashfs_decompressor_create(struct squashfs_sb_info *,
void *)" frees "comp_opts".[show details]
136stream = squashfs_decompressor_create(msblk, comp_opts);
3. Condition "IS_ERR(stream)", taking true branch
137if (IS_ERR(stream))
CID 1130962 (#1 of 1): Double free (USE_AFTER_FREE)4. double_free:
Calling "kfree(void const *)" frees pointer "comp_opts" which has
already been freed.
138kfree(comp_opts);
139
140return stream;
141}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks
2013/11/21 Geyslan Gregório Bem : > 2013/11/20 Al Viro : >> On Wed, Nov 20, 2013 at 09:34:31PM -0300, Geyslan G. Bem wrote: >>> The member 'e_ehsize' that holds the ELF header size is compared >>> with the elfhdr struct size. If not equal, goes out. >>> If 'e_phoff' holds 0 the object has no program header table, so >>> goes out. >>> Ensures the file being loaded has the correct data encoding, checking >>> 'e_ident[EI_DATA]' against 'ELF_DATA'. >>> >>> Besides the checks being in accordance with the ELF Specifications, >>> they increase the binary consistency reducing the use of malformed ones. >> >> This is completely misguided. We are allowed to reject such binaries, >> but what's the point of doing that? > > Viro, First of all, thanks for reply. > > The security (or anti-security) guys are used to mess up with the not checked > header fields for their "benefits": anti-debugging, injection and so on. > > Concerning to 'e_phoff': when it is 0 the check avoids that 'elf_phdr' been > read > from a erroneous offset (ELF header). I know that without this check the > binary > will goes out anyway. But it reduces wasting cpu cycles. > > Regarding 'e_ident[EI_DATA]': that check also prevents a farther code reading > when the binary, although been the correct arch, is compiled with a different > data encoding (MSB vs LSB). > > So checking besides increase the binary consistency, guarantee some mislead > and fewer cpu cycles. > > -- > Regards, > > Geyslan G. Bem > hackingbits.com Another good reason is that ld does reject such binaries (I hex edit one to hold MSB value in header): uzumaki@hb ~ $ /lib/ld-linux-x86-64.so.2 ./a.out ./a.out: error while loading shared libraries: ./a.out: ELF file data encoding not little-endian After zeroing the phoff: uzumaki@hb ~ $ /lib/ld-linux-x86-64.so.2 ./a.out ./a.out: error while loading shared libraries: ./a.out: object file has no loadable segments I really think that is a way to get a more robust binfmt consistency check. What you think? -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks
2013/11/21 Geyslan Gregório Bem geys...@gmail.com: 2013/11/20 Al Viro v...@zeniv.linux.org.uk: On Wed, Nov 20, 2013 at 09:34:31PM -0300, Geyslan G. Bem wrote: The member 'e_ehsize' that holds the ELF header size is compared with the elfhdr struct size. If not equal, goes out. If 'e_phoff' holds 0 the object has no program header table, so goes out. Ensures the file being loaded has the correct data encoding, checking 'e_ident[EI_DATA]' against 'ELF_DATA'. Besides the checks being in accordance with the ELF Specifications, they increase the binary consistency reducing the use of malformed ones. This is completely misguided. We are allowed to reject such binaries, but what's the point of doing that? Viro, First of all, thanks for reply. The security (or anti-security) guys are used to mess up with the not checked header fields for their benefits: anti-debugging, injection and so on. Concerning to 'e_phoff': when it is 0 the check avoids that 'elf_phdr' been read from a erroneous offset (ELF header). I know that without this check the binary will goes out anyway. But it reduces wasting cpu cycles. Regarding 'e_ident[EI_DATA]': that check also prevents a farther code reading when the binary, although been the correct arch, is compiled with a different data encoding (MSB vs LSB). So checking besides increase the binary consistency, guarantee some mislead and fewer cpu cycles. -- Regards, Geyslan G. Bem hackingbits.com Another good reason is that ld does reject such binaries (I hex edit one to hold MSB value in header): uzumaki@hb ~ $ /lib/ld-linux-x86-64.so.2 ./a.out ./a.out: error while loading shared libraries: ./a.out: ELF file data encoding not little-endian After zeroing the phoff: uzumaki@hb ~ $ /lib/ld-linux-x86-64.so.2 ./a.out ./a.out: error while loading shared libraries: ./a.out: object file has no loadable segments I really think that is a way to get a more robust binfmt consistency check. What you think? -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] double free in decompressor.c
Coverity caught double free possibility (CID 1130962).
I can patch this, but I have to know if is correct to free comp_opts
in the function squashfs_decompressor_create() or it had to be done in
the caller. My bet is the caller.
128void *squashfs_decompressor_setup(struct super_block *sb, unsigned
short flags)
129{
130struct squashfs_sb_info *msblk = sb-s_fs_info;
131void *stream, *comp_opts = get_comp_opts(sb, flags);
132
1. Condition IS_ERR(comp_opts), taking false branch
133if (IS_ERR(comp_opts))
134return comp_opts;
135
2. freed_arg: squashfs_decompressor_create(struct squashfs_sb_info *,
void *) frees comp_opts.[show details]
136stream = squashfs_decompressor_create(msblk, comp_opts);
3. Condition IS_ERR(stream), taking true branch
137if (IS_ERR(stream))
CID 1130962 (#1 of 1): Double free (USE_AFTER_FREE)4. double_free:
Calling kfree(void const *) frees pointer comp_opts which has
already been freed.
138kfree(comp_opts);
139
140return stream;
141}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] double free in decompressor.c
2013/11/22 Richard Weinberger richard.weinber...@gmail.com: On Fri, Nov 22, 2013 at 10:50 PM, Geyslan Gregório Bem geys...@gmail.com wrote: Coverity caught double free possibility (CID 1130962). Just wondering, where can one find/verify such CIDs? -- Thanks, //richard Anyone can sign in (https://scan.coverity.com/) and choose an open source project or register one (as maintainer) indeed. After that, configure to receive new defects updates. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] double free in decompressor.c
2013/11/22 Phillip Lougher phil...@lougher.demon.co.uk:
On 22/11/13 21:50, Geyslan Gregório Bem wrote:
Coverity caught double free possibility (CID 1130962).
I can patch this, but I have to know if is correct to free comp_opts
in the function squashfs_decompressor_create() or it had to be done in
the caller. My bet is the caller.
128void *squashfs_decompressor_setup(struct super_block *sb, unsigned
short flags)
129{
130struct squashfs_sb_info *msblk = sb-s_fs_info;
131void *stream, *comp_opts = get_comp_opts(sb, flags);
132
1. Condition IS_ERR(comp_opts), taking false branch
133if (IS_ERR(comp_opts))
134return comp_opts;
135
2. freed_arg: squashfs_decompressor_create(struct squashfs_sb_info *,
void *) frees comp_opts.[show details]
136stream = squashfs_decompressor_create(msblk, comp_opts);
3. Condition IS_ERR(stream), taking true branch
137if (IS_ERR(stream))
FALSE positive.
squashfs_decompressor_create() frees comp_opts only on success.
If IS_ERR(stream) is true, then comp_opts has not been freed by
squashfs_decompressor_create().
Phillip
CID 1130962 (#1 of 1): Double free (USE_AFTER_FREE)4. double_free:
Calling kfree(void const *) frees pointer comp_opts which has
already been freed.
138kfree(comp_opts);
139
140return stream;
141}
Philip, set as false positive in Coverity. Thanks.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks
2013/11/20 Al Viro : > On Wed, Nov 20, 2013 at 09:34:31PM -0300, Geyslan G. Bem wrote: >> The member 'e_ehsize' that holds the ELF header size is compared >> with the elfhdr struct size. If not equal, goes out. >> If 'e_phoff' holds 0 the object has no program header table, so >> goes out. >> Ensures the file being loaded has the correct data encoding, checking >> 'e_ident[EI_DATA]' against 'ELF_DATA'. >> >> Besides the checks being in accordance with the ELF Specifications, >> they increase the binary consistency reducing the use of malformed ones. > > This is completely misguided. We are allowed to reject such binaries, > but what's the point of doing that? Viro, First of all, thanks for reply. The security (or anti-security) guys are used to mess up with the not checked header fields for their "benefits": anti-debugging, injection and so on. Concerning to 'e_phoff': when it is 0 the check avoids that 'elf_phdr' been read from a erroneous offset (ELF header). I know that without this check the binary will goes out anyway. But it reduces wasting cpu cycles. Regarding 'e_ident[EI_DATA]': that check also prevents a farther code reading when the binary, although been the correct arch, is compiled with a different data encoding (MSB vs LSB). So checking besides increase the binary consistency, guarantee some mislead and fewer cpu cycles. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks
2013/11/20 Al Viro v...@zeniv.linux.org.uk: On Wed, Nov 20, 2013 at 09:34:31PM -0300, Geyslan G. Bem wrote: The member 'e_ehsize' that holds the ELF header size is compared with the elfhdr struct size. If not equal, goes out. If 'e_phoff' holds 0 the object has no program header table, so goes out. Ensures the file being loaded has the correct data encoding, checking 'e_ident[EI_DATA]' against 'ELF_DATA'. Besides the checks being in accordance with the ELF Specifications, they increase the binary consistency reducing the use of malformed ones. This is completely misguided. We are allowed to reject such binaries, but what's the point of doing that? Viro, First of all, thanks for reply. The security (or anti-security) guys are used to mess up with the not checked header fields for their benefits: anti-debugging, injection and so on. Concerning to 'e_phoff': when it is 0 the check avoids that 'elf_phdr' been read from a erroneous offset (ELF header). I know that without this check the binary will goes out anyway. But it reduces wasting cpu cycles. Regarding 'e_ident[EI_DATA]': that check also prevents a farther code reading when the binary, although been the correct arch, is compiled with a different data encoding (MSB vs LSB). So checking besides increase the binary consistency, guarantee some mislead and fewer cpu cycles. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: binfmt_elf: Add two ELF header consistency checks
2013/11/20 Geyslan G. Bem : > The member 'e_ehsize' that holds the ELF header size is compared > with the elfhdr struct size. If not equal, goes out. > If 'e_phoff' holds 0 the object has no program header table, so > goes out. > > Increasing the binary consistency reduces the use of malformed ones. > > Both checks are in accordance with the ELF Specifications. > > Signed-off-by: Geyslan G. Bem > --- > fs/binfmt_elf.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 571a423..3c08f96 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -603,6 +603,8 @@ static int load_elf_binary(struct linux_binprm *bprm) > if (memcmp(loc->elf_ex.e_ident, ELFMAG, SELFMAG) != 0) > goto out; > > + if (loc->elf_ex.e_ehsize != sizeof(struct elfhdr)) > + goto out; > if (loc->elf_ex.e_type != ET_EXEC && loc->elf_ex.e_type != ET_DYN) > goto out; > if (!elf_check_arch(>elf_ex)) > @@ -611,6 +613,8 @@ static int load_elf_binary(struct linux_binprm *bprm) > goto out; > > /* Now read in all of the header information */ > + if (loc->elf_ex.e_phoff == 0) > + goto out; > if (loc->elf_ex.e_phentsize != sizeof(struct elf_phdr)) > goto out; > if (loc->elf_ex.e_phnum < 1 || > -- > 1.8.4.2 > PATCH v2 Sent: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: binfmt_elf: Add two ELF header consistency checks
2013/11/20 Geyslan G. Bem geys...@gmail.com: The member 'e_ehsize' that holds the ELF header size is compared with the elfhdr struct size. If not equal, goes out. If 'e_phoff' holds 0 the object has no program header table, so goes out. Increasing the binary consistency reduces the use of malformed ones. Both checks are in accordance with the ELF Specifications. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- fs/binfmt_elf.c | 4 1 file changed, 4 insertions(+) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 571a423..3c08f96 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -603,6 +603,8 @@ static int load_elf_binary(struct linux_binprm *bprm) if (memcmp(loc-elf_ex.e_ident, ELFMAG, SELFMAG) != 0) goto out; + if (loc-elf_ex.e_ehsize != sizeof(struct elfhdr)) + goto out; if (loc-elf_ex.e_type != ET_EXEC loc-elf_ex.e_type != ET_DYN) goto out; if (!elf_check_arch(loc-elf_ex)) @@ -611,6 +613,8 @@ static int load_elf_binary(struct linux_binprm *bprm) goto out; /* Now read in all of the header information */ + if (loc-elf_ex.e_phoff == 0) + goto out; if (loc-elf_ex.e_phentsize != sizeof(struct elf_phdr)) goto out; if (loc-elf_ex.e_phnum 1 || -- 1.8.4.2 PATCH v2 Sent: [PATCH v2] fs: binfmt_elf: Add ELF header consistency checks -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Fwd: [PATCH] fs: binfmt_elf: add ELF reference in header comment
This patch add the specification "System V Application Binary Interface" reference to header comment. Signed-off-by: Geyslan G. Bem --- fs/binfmt_elf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 571a423..18938f9 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -2,9 +2,10 @@ * linux/fs/binfmt_elf.c * * These are the functions used to load ELF format executables as used - * on SVr4 machines. Information on the format may be found in the book - * "UNIX SYSTEM V RELEASE 4 Programmers Guide: Ansi C and Programming Support - * Tools". + * on SVr4 machines. Information on the format may be found in the + * specification "System V Application Binary Interface" provided by SCO, + * as well as in the book "UNIX SYSTEM V RELEASE 4 Programmers Guide: Ansi C + * and Programming Support Tools". * * Copyright 1993, 1994: Eric Youngdale (er...@cais.com). */ -- 1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Fwd: [PATCH] fs: binfmt_elf: add ELF reference in header comment
This patch add the specification System V Application Binary Interface reference to header comment. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- fs/binfmt_elf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 571a423..18938f9 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -2,9 +2,10 @@ * linux/fs/binfmt_elf.c * * These are the functions used to load ELF format executables as used - * on SVr4 machines. Information on the format may be found in the book - * UNIX SYSTEM V RELEASE 4 Programmers Guide: Ansi C and Programming Support - * Tools. + * on SVr4 machines. Information on the format may be found in the + * specification System V Application Binary Interface provided by SCO, + * as well as in the book UNIX SYSTEM V RELEASE 4 Programmers Guide: Ansi C + * and Programming Support Tools. * * Copyright 1993, 1994: Eric Youngdale (er...@cais.com). */ -- 1.8.4.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 Geyslan Gregório Bem :
> 2013/11/18 James Bottomley :
>> On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
>>> 2013/11/18 James Bottomley :
>>> > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>>> >> 2013/11/17 James Bottomley :
>>> >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>>> >> >> 2013/11/17 James Bottomley :
>>> >> >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
>>> >> >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID'
>>> >> >> >> and
>>> >> >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
>>> >> >> >> necessary.
>>> >> >> >
>>> >> >> > You pointlessly renamed a variable, which makes the diff hard to
>>> >> >> > read.
>>> >> >> > Please don't do that.
>>> >> >>
>>> >> >> Ok, I can agree. 'len' means length? What is returned in case of non
>>> >> >> error?
>>> >> >
>>> >> > it returns the length of buf written to or negative error.
>>> >> >
>>> >> >> > You missed the fact that the passed in pointer is unmodified if
>>> >> >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and
>>> >> >> > would
>>> >> >> > oops.
>>> >> >> >
>>> >> >> > There's no need for a goto; len = -EINVAL; does everything that's
>>> >> >> > needed.
>>> >> >>
>>> >> >> Well, that is a coverity catch. CID 1128954. Check it.
>>> >> >
>>> >> > I didn't say coverity was wrong, I said your patch was (well not wrong,
>>> >> > just over complex and incomplete). This is the way to fix both
>>> >> > problems.
>>> >> >
>>> >> > James
>>> >> >
>>> >> > ---
>>> >> >
>>> >> > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
>>> >> > b/drivers/scsi/be2iscsi/be_iscsi.c
>>> >> > index ffadbee..9dcbdfa 100644
>>> >> > --- a/drivers/scsi/be2iscsi/be_iscsi.c
>>> >> > +++ b/drivers/scsi/be2iscsi/be_iscsi.c
>>> >> > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct
>>> >> > beiscsi_hba *phba,
>>> >> > ip_type = BE2_IPV6;
>>> >>
>>> >> James, this approach will not prevent the leakage.
>>> >
>>> > I don't see why not. The -EINVAL case goes through the kfree() now too,
>>> > no?
>>>
>>> I'm refering to the removal of kfree in your suggestion.
>>
>> That's the second bug I pointed out via code inspection. If the
>> function returns an error (any non zero return) then the pointer isn't
>> altered, so we return without the free. It's a standard error pattern.
>
> Ok. So kfree is useless until the code go to the function bail. Right?
>
>>
>>> >
>>> >> We can initialize the if_info with NULL and always kfree it without
>>> >> to care about junk.
>>> >
>>> > Why? Error return means no allocation.
>>> Setting if_info to NULL allow to kfree without concerns.
>>>
>>> Eg.:
>>>
>>> - struct be_cmd_get_if_info_resp *if_info;
>>> + struct be_cmd_get_if_info_resp *if_info = NULL;
>>>
>>> ...
>>>
>>> + if (len)
>>> + goto out;
>>>
>>> ...
>>>
>>> - if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
>>> - return -EINVAL;
>>> + if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
>>> + len = -EINVAL;
>>> + goto out;
>>> + }
>>
>> What's the point of that? Just removing the goto out; has the code
>> going to the same place because of the break below.
>>
>> James
>>
>>
>
> I agree whit you that the code goes to same place with or without goto
> out. It's just a pattern to simplify future changes in the function.
>
> But I can remove it if you want.
>
>
> --
> Regards,
>
> Geyslan G. Bem
> hackingbits.com
Done:
[PATCH v2] scsi: be_iscsi: fix possible memory leak
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley :
> On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/18 James Bottomley :
>> > On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>> >> 2013/11/17 James Bottomley :
>> >> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> >> >> 2013/11/17 James Bottomley :
>> >> >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
>> >> >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
>> >> >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
>> >> >> >> necessary.
>> >> >> >
>> >> >> > You pointlessly renamed a variable, which makes the diff hard to
>> >> >> > read.
>> >> >> > Please don't do that.
>> >> >>
>> >> >> Ok, I can agree. 'len' means length? What is returned in case of non
>> >> >> error?
>> >> >
>> >> > it returns the length of buf written to or negative error.
>> >> >
>> >> >> > You missed the fact that the passed in pointer is unmodified if
>> >> >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and
>> >> >> > would
>> >> >> > oops.
>> >> >> >
>> >> >> > There's no need for a goto; len = -EINVAL; does everything that's
>> >> >> > needed.
>> >> >>
>> >> >> Well, that is a coverity catch. CID 1128954. Check it.
>> >> >
>> >> > I didn't say coverity was wrong, I said your patch was (well not wrong,
>> >> > just over complex and incomplete). This is the way to fix both
>> >> > problems.
>> >> >
>> >> > James
>> >> >
>> >> > ---
>> >> >
>> >> > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
>> >> > b/drivers/scsi/be2iscsi/be_iscsi.c
>> >> > index ffadbee..9dcbdfa 100644
>> >> > --- a/drivers/scsi/be2iscsi/be_iscsi.c
>> >> > +++ b/drivers/scsi/be2iscsi/be_iscsi.c
>> >> > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct
>> >> > beiscsi_hba *phba,
>> >> > ip_type = BE2_IPV6;
>> >>
>> >> James, this approach will not prevent the leakage.
>> >
>> > I don't see why not. The -EINVAL case goes through the kfree() now too,
>> > no?
>>
>> I'm refering to the removal of kfree in your suggestion.
>
> That's the second bug I pointed out via code inspection. If the
> function returns an error (any non zero return) then the pointer isn't
> altered, so we return without the free. It's a standard error pattern.
Ok. So kfree is useless until the code go to the function bail. Right?
>
>> >
>> >> We can initialize the if_info with NULL and always kfree it without
>> >> to care about junk.
>> >
>> > Why? Error return means no allocation.
>> Setting if_info to NULL allow to kfree without concerns.
>>
>> Eg.:
>>
>> - struct be_cmd_get_if_info_resp *if_info;
>> + struct be_cmd_get_if_info_resp *if_info = NULL;
>>
>> ...
>>
>> + if (len)
>> + goto out;
>>
>> ...
>>
>> - if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
>> - return -EINVAL;
>> + if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
>> + len = -EINVAL;
>> + goto out;
>> + }
>
> What's the point of that? Just removing the goto out; has the code
> going to the same place because of the break below.
>
> James
>
>
I agree whit you that the code goes to same place with or without goto
out. It's just a pattern to simplify future changes in the function.
But I can remove it if you want.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley :
> On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/17 James Bottomley :
>> > On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> >> 2013/11/17 James Bottomley :
>> >> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
>> >> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
>> >> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
>> >> >> necessary.
>> >> >
>> >> > You pointlessly renamed a variable, which makes the diff hard to read.
>> >> > Please don't do that.
>> >>
>> >> Ok, I can agree. 'len' means length? What is returned in case of non
>> >> error?
>> >
>> > it returns the length of buf written to or negative error.
>> >
>> >> > You missed the fact that the passed in pointer is unmodified if
>> >> > mgmt_get_if_info() returns non zero, so the kfree frees junk and would
>> >> > oops.
>> >> >
>> >> > There's no need for a goto; len = -EINVAL; does everything that's
>> >> > needed.
>> >>
>> >> Well, that is a coverity catch. CID 1128954. Check it.
>> >
>> > I didn't say coverity was wrong, I said your patch was (well not wrong,
>> > just over complex and incomplete). This is the way to fix both
>> > problems.
>> >
>> > James
>> >
>> > ---
>> >
>> > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
>> > b/drivers/scsi/be2iscsi/be_iscsi.c
>> > index ffadbee..9dcbdfa 100644
>> > --- a/drivers/scsi/be2iscsi/be_iscsi.c
>> > +++ b/drivers/scsi/be2iscsi/be_iscsi.c
>> > @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba
>> > *phba,
>> > ip_type = BE2_IPV6;
>>
>> James, this approach will not prevent the leakage.
>
> I don't see why not. The -EINVAL case goes through the kfree() now too,
> no?
I'm refering to the removal of kfree in your suggestion.
>
>> We can initialize the if_info with NULL and always kfree it without
>> to care about junk.
>
> Why? Error return means no allocation.
Setting if_info to NULL allow to kfree without concerns.
Eg.:
- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;
...
+ if (len)
+ goto out;
...
- if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
...
case ISCSI_NET_PARAM_VLAN_PRIORITY:
- if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
+out:
kfree(if_info);
return len;
Thus, if if_info remains NULL (was not allocated after
mgmt_get_if_info call) kfree returns without oops. And we can
centralize the freeing in the out regarding chapter 7 of coding style
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n386).
What do you think?
>
> James
>
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/18 Caizhiyong : > From: Cai Zhiyong > Date: Mon, 18 Nov 2013 20:21:49 +0800 > Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value > > This patch give a warning when calling regmap_register_patch with > parameter num_regs <= 0. > > When the num_regs parameter is zero and krealloc doesn't fail, > then the code would return an uninitialized value. However, > calling this function with num_regs == 0, would be a waste as it > essentially does nothing. > > Signed-off-by: Cai Zhiyong Reviewed-by: Geyslan G. Bem -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/18 Caizhiyong caizhiy...@hisilicon.com: From: Cai Zhiyong caizhiy...@huawei.com Date: Mon, 18 Nov 2013 20:21:49 +0800 Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value This patch give a warning when calling regmap_register_patch with parameter num_regs = 0. When the num_regs parameter is zero and krealloc doesn't fail, then the code would return an uninitialized value. However, calling this function with num_regs == 0, would be a waste as it essentially does nothing. Signed-off-by: Cai Zhiyong caizhiy...@huawei.com Reviewed-by: Geyslan G. Bem geys...@gmail.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
necessary.
You pointlessly renamed a variable, which makes the diff hard to read.
Please don't do that.
Ok, I can agree. 'len' means length? What is returned in case of non
error?
it returns the length of buf written to or negative error.
You missed the fact that the passed in pointer is unmodified if
mgmt_get_if_info() returns non zero, so the kfree frees junk and would
oops.
There's no need for a goto; len = -EINVAL; does everything that's
needed.
Well, that is a coverity catch. CID 1128954. Check it.
I didn't say coverity was wrong, I said your patch was (well not wrong,
just over complex and incomplete). This is the way to fix both
problems.
James
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba
*phba,
ip_type = BE2_IPV6;
James, this approach will not prevent the leakage.
I don't see why not. The -EINVAL case goes through the kfree() now too,
no?
I'm refering to the removal of kfree in your suggestion.
We can initialize the if_info with NULL and always kfree it without
to care about junk.
Why? Error return means no allocation.
Setting if_info to NULL allow to kfree without concerns.
Eg.:
- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;
...
+ if (len)
+ goto out;
...
- if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
...
case ISCSI_NET_PARAM_VLAN_PRIORITY:
- if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
+out:
kfree(if_info);
return len;
Thus, if if_info remains NULL (was not allocated after
mgmt_get_if_info call) kfree returns without oops. And we can
centralize the freeing in the out regarding chapter 7 of coding style
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n386).
What do you think?
James
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
necessary.
You pointlessly renamed a variable, which makes the diff hard to
read.
Please don't do that.
Ok, I can agree. 'len' means length? What is returned in case of non
error?
it returns the length of buf written to or negative error.
You missed the fact that the passed in pointer is unmodified if
mgmt_get_if_info() returns non zero, so the kfree frees junk and
would
oops.
There's no need for a goto; len = -EINVAL; does everything that's
needed.
Well, that is a coverity catch. CID 1128954. Check it.
I didn't say coverity was wrong, I said your patch was (well not wrong,
just over complex and incomplete). This is the way to fix both
problems.
James
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct
beiscsi_hba *phba,
ip_type = BE2_IPV6;
James, this approach will not prevent the leakage.
I don't see why not. The -EINVAL case goes through the kfree() now too,
no?
I'm refering to the removal of kfree in your suggestion.
That's the second bug I pointed out via code inspection. If the
function returns an error (any non zero return) then the pointer isn't
altered, so we return without the free. It's a standard error pattern.
Ok. So kfree is useless until the code go to the function bail. Right?
We can initialize the if_info with NULL and always kfree it without
to care about junk.
Why? Error return means no allocation.
Setting if_info to NULL allow to kfree without concerns.
Eg.:
- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;
...
+ if (len)
+ goto out;
...
- if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
What's the point of that? Just removing the goto out; has the code
going to the same place because of the break below.
James
I agree whit you that the code goes to same place with or without goto
out. It's just a pattern to simplify future changes in the function.
But I can remove it if you want.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/18 Geyslan Gregório Bem geys...@gmail.com:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Mon, 2013-11-18 at 14:18 -0200, Geyslan Gregório Bem wrote:
2013/11/18 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 23:12 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID'
and
'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
necessary.
You pointlessly renamed a variable, which makes the diff hard to
read.
Please don't do that.
Ok, I can agree. 'len' means length? What is returned in case of non
error?
it returns the length of buf written to or negative error.
You missed the fact that the passed in pointer is unmodified if
mgmt_get_if_info() returns non zero, so the kfree frees junk and
would
oops.
There's no need for a goto; len = -EINVAL; does everything that's
needed.
Well, that is a coverity catch. CID 1128954. Check it.
I didn't say coverity was wrong, I said your patch was (well not wrong,
just over complex and incomplete). This is the way to fix both
problems.
James
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct
beiscsi_hba *phba,
ip_type = BE2_IPV6;
James, this approach will not prevent the leakage.
I don't see why not. The -EINVAL case goes through the kfree() now too,
no?
I'm refering to the removal of kfree in your suggestion.
That's the second bug I pointed out via code inspection. If the
function returns an error (any non zero return) then the pointer isn't
altered, so we return without the free. It's a standard error pattern.
Ok. So kfree is useless until the code go to the function bail. Right?
We can initialize the if_info with NULL and always kfree it without
to care about junk.
Why? Error return means no allocation.
Setting if_info to NULL allow to kfree without concerns.
Eg.:
- struct be_cmd_get_if_info_resp *if_info;
+ struct be_cmd_get_if_info_resp *if_info = NULL;
...
+ if (len)
+ goto out;
...
- if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE) {
+ len = -EINVAL;
+ goto out;
+ }
What's the point of that? Just removing the goto out; has the code
going to the same place because of the break below.
James
I agree whit you that the code goes to same place with or without goto
out. It's just a pattern to simplify future changes in the function.
But I can remove it if you want.
--
Regards,
Geyslan G. Bem
hackingbits.com
Done:
[PATCH v2] scsi: be_iscsi: fix possible memory leak
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley :
> On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
>> 2013/11/17 James Bottomley :
>> > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
>> >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
>> >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
>> >> necessary.
>> >
>> > You pointlessly renamed a variable, which makes the diff hard to read.
>> > Please don't do that.
>>
>> Ok, I can agree. 'len' means length? What is returned in case of non
>> error?
>
> it returns the length of buf written to or negative error.
>
>> > You missed the fact that the passed in pointer is unmodified if
>> > mgmt_get_if_info() returns non zero, so the kfree frees junk and would
>> > oops.
>> >
>> > There's no need for a goto; len = -EINVAL; does everything that's
>> > needed.
>>
>> Well, that is a coverity catch. CID 1128954. Check it.
>
> I didn't say coverity was wrong, I said your patch was (well not wrong,
> just over complex and incomplete). This is the way to fix both
> problems.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
> b/drivers/scsi/be2iscsi/be_iscsi.c
> index ffadbee..9dcbdfa 100644
> --- a/drivers/scsi/be2iscsi/be_iscsi.c
> +++ b/drivers/scsi/be2iscsi/be_iscsi.c
> @@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba
> *phba,
> ip_type = BE2_IPV6;
James, this approach will not prevent the leakage. We can initialize
the if_info with NULL and always kfree it without to care about junk.
>
> len = mgmt_get_if_info(phba, ip_type, _info);
> - if (len) {
> - kfree(if_info);
> + if (len)
> return len;
> - }
>
> switch (param) {
> case ISCSI_NET_PARAM_IPV4_ADDR:
> @@ -569,7 +567,7 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
> break;
> case ISCSI_NET_PARAM_VLAN_ID:
> if (if_info->vlan_priority == BEISCSI_VLAN_DISABLE)
> - return -EINVAL;
> + len = -EINVAL;
> else
> len = sprintf(buf, "%d\n",
> (if_info->vlan_priority &
>
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley : > On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: >> This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and >> 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when >> necessary. > > You pointlessly renamed a variable, which makes the diff hard to read. > Please don't do that. Ok, I can agree. 'len' means length? What is returned in case of non error? > > You missed the fact that the passed in pointer is unmodified if > mgmt_get_if_info() returns non zero, so the kfree frees junk and would > oops. > > There's no need for a goto; len = -EINVAL; does everything that's > needed. Well, that is a coverity catch. CID 1128954. Check it. > > James > > Thanks. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] possible null dereference in 'pm121_create_sys_fans()'
Hello Benjamin,
In file 'windfarm_pm121'.c:
If this branch is true:
if (param == NULL) {
printk(KERN_WARNING "pm121: %s fan config not found "
" for this machine model\n",
loop_names[loop_id]);
goto fail;
}
control that is NULL will suffer a dereference attempt:
fail:
/* note that this is not optimal since another loop may still
control the same control */
printk(KERN_WARNING "pm121: failed to set up %s loop "
"setting \"%s\" to max speed.\n",
loop_names[loop_id], control->name);
Reported-by: Geyslan G. Bem
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] possible null dereference in 'pm121_create_sys_fans()'
Hello Benjamin,
In file 'windfarm_pm121'.c:
If this branch is true:
if (param == NULL) {
printk(KERN_WARNING pm121: %s fan config not found
for this machine model\n,
loop_names[loop_id]);
goto fail;
}
control that is NULL will suffer a dereference attempt:
fail:
/* note that this is not optimal since another loop may still
control the same control */
printk(KERN_WARNING pm121: failed to set up %s loop
setting \%s\ to max speed.\n,
loop_names[loop_id], control-name);
Reported-by: Geyslan G. Bem geys...@gmail.com
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com: On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote: This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and 'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when necessary. You pointlessly renamed a variable, which makes the diff hard to read. Please don't do that. Ok, I can agree. 'len' means length? What is returned in case of non error? You missed the fact that the passed in pointer is unmodified if mgmt_get_if_info() returns non zero, so the kfree frees junk and would oops. There's no need for a goto; len = -EINVAL; does everything that's needed. Well, that is a coverity catch. CID 1128954. Check it. James Thanks. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] scsi: be_iscsi: fix possible memory leak and refactor code
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 19:09 -0200, Geyslan Gregório Bem wrote:
2013/11/17 James Bottomley james.bottom...@hansenpartnership.com:
On Sun, 2013-11-17 at 15:51 -0300, Geyslan G. Bem wrote:
This patch fix memory leakage in cases 'ISCSI_NET_PARAM_VLAN_ID' and
'ISCSI_NET_PARAM_VLAN_PRIORITY' and refactors code 'going out' when
necessary.
You pointlessly renamed a variable, which makes the diff hard to read.
Please don't do that.
Ok, I can agree. 'len' means length? What is returned in case of non
error?
it returns the length of buf written to or negative error.
You missed the fact that the passed in pointer is unmodified if
mgmt_get_if_info() returns non zero, so the kfree frees junk and would
oops.
There's no need for a goto; len = -EINVAL; does everything that's
needed.
Well, that is a coverity catch. CID 1128954. Check it.
I didn't say coverity was wrong, I said your patch was (well not wrong,
just over complex and incomplete). This is the way to fix both
problems.
James
---
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
b/drivers/scsi/be2iscsi/be_iscsi.c
index ffadbee..9dcbdfa 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -541,10 +541,8 @@ static int be2iscsi_get_if_param(struct beiscsi_hba
*phba,
ip_type = BE2_IPV6;
James, this approach will not prevent the leakage. We can initialize
the if_info with NULL and always kfree it without to care about junk.
len = mgmt_get_if_info(phba, ip_type, if_info);
- if (len) {
- kfree(if_info);
+ if (len)
return len;
- }
switch (param) {
case ISCSI_NET_PARAM_IPV4_ADDR:
@@ -569,7 +567,7 @@ static int be2iscsi_get_if_param(struct beiscsi_hba *phba,
break;
case ISCSI_NET_PARAM_VLAN_ID:
if (if_info-vlan_priority == BEISCSI_VLAN_DISABLE)
- return -EINVAL;
+ len = -EINVAL;
else
len = sprintf(buf, %d\n,
(if_info-vlan_priority
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] drivers/scsi/dc395x.c - msgin_qtag()
Hi guys,
In the function msgin_qtag() [line 2632], this dereference was intentional?
static struct ScsiReqBlk *msgin_qtag(struct AdapterCtlBlk *acb,
struct DeviceCtlBlk *dcb, u8 tag)
{
struct ScsiReqBlk *srb = NULL;
struct ScsiReqBlk *i;
dprintkdbg(DBG_0, "msgin_qtag: (0x%p) tag=%i srb=%p\n",
srb->cmd, tag, srb);
...
There is a srb (NULL) dereference in the dprintkdbg() parameteres.
If not, what approach do you suggest me for a patch?
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Mark Brown : > On Fri, Nov 15, 2013 at 01:52:19PM -0200, Geyslan Gregório Bem wrote: > >> + if (WARN_ONCE(num_regs <= 0, "number of registers [%d] must be >> major than 0", num_regs)) >> + return 0; > > "must be more than" but yes. Tks. Sorry for my poor english. :D -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Levente Kurusa : > 2013-11-15 14:00 keltezéssel, Geyslan Gregório Bem írta: >> 2013/11/15 Caizhiyong : >>> From: Cai Zhiyong >>> Date: Mon, 11 Nov 2013 19:26:14 +0800 >>> Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value >>> >>> This patch give a warning when calling regmap_register_patch with >>> parameter num_regs <= 0. >>> >>> When the num_regs parameter is zero and krealloc doesn't fail, >>> then the code would return an uninitialized value. However, >>> calling this function with num_regs == 0, would be a waste as it >>> essentially does nothing. >>> >>> Signed-off-by: Cai Zhiyong >> Reviewed-by: Geyslan G. Bem >> >> Seems a good approach. >> > > The patch doesn't apply. After manually applying this it > threw a compilation error: > > drivers/base/regmap/regmap.c: In function ‘regmap_register_patch’: > drivers/base/regmap/regmap.c:2176:6: error: expected expression before ‘)’ > token > > > Cai, > You have missed the second parameter of WARN_ONCE(). > Please append what you'd prefer to have outputten once the condition > evaluates to true. > > -- > Regards, > Levente Kurusa Cai, Levente, Maybe this: + if (WARN_ONCE(num_regs <= 0, "number of registers [%d] must be major than 0", num_regs)) + return 0; + -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Caizhiyong : > From: Cai Zhiyong > Date: Mon, 11 Nov 2013 19:26:14 +0800 > Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value > > This patch give a warning when calling regmap_register_patch with > parameter num_regs <= 0. > > When the num_regs parameter is zero and krealloc doesn't fail, > then the code would return an uninitialized value. However, > calling this function with num_regs == 0, would be a waste as it > essentially does nothing. > > Signed-off-by: Cai Zhiyong Reviewed-by: Geyslan G. Bem Seems a good approach. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Caizhiyong caizhiy...@hisilicon.com: From: Cai Zhiyong caizhiy...@huawei.com Date: Mon, 11 Nov 2013 19:26:14 +0800 Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value This patch give a warning when calling regmap_register_patch with parameter num_regs = 0. When the num_regs parameter is zero and krealloc doesn't fail, then the code would return an uninitialized value. However, calling this function with num_regs == 0, would be a waste as it essentially does nothing. Signed-off-by: Cai Zhiyong caizhiy...@huawei.com Reviewed-by: Geyslan G. Bem geys...@gmail.com Seems a good approach. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Levente Kurusa le...@linux.com: 2013-11-15 14:00 keltezéssel, Geyslan Gregório Bem írta: 2013/11/15 Caizhiyong caizhiy...@hisilicon.com: From: Cai Zhiyong caizhiy...@huawei.com Date: Mon, 11 Nov 2013 19:26:14 +0800 Subject: [PATCH] regmap: Fix 'ret' would return an uninitialized value This patch give a warning when calling regmap_register_patch with parameter num_regs = 0. When the num_regs parameter is zero and krealloc doesn't fail, then the code would return an uninitialized value. However, calling this function with num_regs == 0, would be a waste as it essentially does nothing. Signed-off-by: Cai Zhiyong caizhiy...@huawei.com Reviewed-by: Geyslan G. Bem geys...@gmail.com Seems a good approach. The patch doesn't apply. After manually applying this it threw a compilation error: drivers/base/regmap/regmap.c: In function ‘regmap_register_patch’: drivers/base/regmap/regmap.c:2176:6: error: expected expression before ‘)’ token Cai, You have missed the second parameter of WARN_ONCE(). Please append what you'd prefer to have outputten once the condition evaluates to true. -- Regards, Levente Kurusa Cai, Levente, Maybe this: + if (WARN_ONCE(num_regs = 0, number of registers [%d] must be major than 0, num_regs)) + return 0; + -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: Fix 'ret' would return an uninitialized value
2013/11/15 Mark Brown broo...@kernel.org: On Fri, Nov 15, 2013 at 01:52:19PM -0200, Geyslan Gregório Bem wrote: + if (WARN_ONCE(num_regs = 0, number of registers [%d] must be major than 0, num_regs)) + return 0; must be more than but yes. Tks. Sorry for my poor english. :D -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] drivers/scsi/dc395x.c - msgin_qtag()
Hi guys,
In the function msgin_qtag() [line 2632], this dereference was intentional?
static struct ScsiReqBlk *msgin_qtag(struct AdapterCtlBlk *acb,
struct DeviceCtlBlk *dcb, u8 tag)
{
struct ScsiReqBlk *srb = NULL;
struct ScsiReqBlk *i;
dprintkdbg(DBG_0, msgin_qtag: (0x%p) tag=%i srb=%p\n,
srb-cmd, tag, srb);
...
There is a srb (NULL) dereference in the dprintkdbg() parameteres.
If not, what approach do you suggest me for a patch?
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net/hsr: Fix possible leak in 'hsr_get_node_status()'
2013/11/14 Geyslan G. Bem : > If 'hsr_get_node_data()' returns error, going directly to 'fail' label > doesn't free the memory pointed by 'skb_out'. > > Signed-off-by: Geyslan G. Bem > --- > net/hsr/hsr_netlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c > index 4e66bf6..aef8429 100644 > --- a/net/hsr/hsr_netlink.c > +++ b/net/hsr/hsr_netlink.c > @@ -249,7 +249,7 @@ static int hsr_get_node_status(struct sk_buff *skb_in, > struct genl_info *info) > _node_if2_age, > _node_if2_seq); > if (res < 0) > - goto fail; > + goto nla_put_failure; > > res = nla_put(skb_out, HSR_A_NODE_ADDR, ETH_ALEN, > > nla_data(info->attrs[HSR_A_NODE_ADDR])); > -- > 1.8.4.2 > It's a Coverity catch: CID 1128855. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ecryptfs: Fix explicit null dereference
2013/11/14 Tyler Hicks : > On 2013-11-14 17:58:40, Geyslan Gregório Bem wrote: >> 2013/11/14 Tyler Hicks : >> > On 2013-11-14 15:42:14, Geyslan G. Bem wrote: >> >> If the condition 'ecryptfs_file_to_private(file)' takes false branch >> >> lower_file is dereferenced when NULL. >> >> >> >> Caught by Coverity: CIDs 1128834 and 1128833. >> >> >> >> Signed-off-by: Geyslan G. Bem >> >> --- >> > >> > Hello - Smatch picked up on this earlier in week and Dan analyzed the >> > situation here: >> > >> > http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441 >> > >> > I agree with his assessment and proposed the following patch: >> > >> > http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442 >> > >> > It makes Smatch happy and it should also make Coverity happy. >> > >> > Tyler >> >> True. Disregard mine. >> >> Thanks Tyler. > > Thank you! Can I add your Reviewed-by: tag to that patch prior to > pushing it to my next branch? > > Tyler > Sure, Tyler. You can! :) Thanks again. >> >> > >> >> fs/ecryptfs/file.c | 12 >> >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c >> >> index 2229a74..1c0403a 100644 >> >> --- a/fs/ecryptfs/file.c >> >> +++ b/fs/ecryptfs/file.c >> >> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned >> >> int cmd, unsigned long arg) >> >> struct file *lower_file = NULL; >> >> long rc = -ENOTTY; >> >> >> >> - if (ecryptfs_file_to_private(file)) >> >> - lower_file = ecryptfs_file_to_lower(file); >> >> + if (!ecryptfs_file_to_private(file)) >> >> + goto out; >> >> + lower_file = ecryptfs_file_to_lower(file); >> >> if (lower_file->f_op->unlocked_ioctl) >> >> rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg); >> >> +out: >> >> return rc; >> >> } >> >> >> >> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned >> >> int cmd, unsigned long arg) >> >> struct file *lower_file = NULL; >> >> long rc = -ENOIOCTLCMD; >> >> >> >> - if (ecryptfs_file_to_private(file)) >> >> - lower_file = ecryptfs_file_to_lower(file); >> >> + if (!ecryptfs_file_to_private(file)) >> >> + goto out; >> >> + lower_file = ecryptfs_file_to_lower(file); >> >> if (lower_file->f_op && lower_file->f_op->compat_ioctl) >> >> rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg); >> >> +out: >> >> return rc; >> >> } >> >> #endif >> >> -- >> >> 1.8.4.2 >> >> >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in >> >> the body of a message to majord...@vger.kernel.org >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Regards, >> >> Geyslan G. Bem >> hackingbits.com >> -- >> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ecryptfs: Fix explicit null dereference
2013/11/14 Tyler Hicks : > On 2013-11-14 15:42:14, Geyslan G. Bem wrote: >> If the condition 'ecryptfs_file_to_private(file)' takes false branch >> lower_file is dereferenced when NULL. >> >> Caught by Coverity: CIDs 1128834 and 1128833. >> >> Signed-off-by: Geyslan G. Bem >> --- > > Hello - Smatch picked up on this earlier in week and Dan analyzed the > situation here: > > http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441 > > I agree with his assessment and proposed the following patch: > > http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442 > > It makes Smatch happy and it should also make Coverity happy. > > Tyler True. Disregard mine. Thanks Tyler. > >> fs/ecryptfs/file.c | 12 >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c >> index 2229a74..1c0403a 100644 >> --- a/fs/ecryptfs/file.c >> +++ b/fs/ecryptfs/file.c >> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned >> int cmd, unsigned long arg) >> struct file *lower_file = NULL; >> long rc = -ENOTTY; >> >> - if (ecryptfs_file_to_private(file)) >> - lower_file = ecryptfs_file_to_lower(file); >> + if (!ecryptfs_file_to_private(file)) >> + goto out; >> + lower_file = ecryptfs_file_to_lower(file); >> if (lower_file->f_op->unlocked_ioctl) >> rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg); >> +out: >> return rc; >> } >> >> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int >> cmd, unsigned long arg) >> struct file *lower_file = NULL; >> long rc = -ENOIOCTLCMD; >> >> - if (ecryptfs_file_to_private(file)) >> - lower_file = ecryptfs_file_to_lower(file); >> + if (!ecryptfs_file_to_private(file)) >> + goto out; >> + lower_file = ecryptfs_file_to_lower(file); >> if (lower_file->f_op && lower_file->f_op->compat_ioctl) >> rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg); >> +out: >> return rc; >> } >> #endif >> -- >> 1.8.4.2 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ecryptfs: Fix explicit null dereference
2013/11/14 Tyler Hicks tyhi...@canonical.com: On 2013-11-14 15:42:14, Geyslan G. Bem wrote: If the condition 'ecryptfs_file_to_private(file)' takes false branch lower_file is dereferenced when NULL. Caught by Coverity: CIDs 1128834 and 1128833. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- Hello - Smatch picked up on this earlier in week and Dan analyzed the situation here: http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441 I agree with his assessment and proposed the following patch: http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442 It makes Smatch happy and it should also make Coverity happy. Tyler True. Disregard mine. Thanks Tyler. fs/ecryptfs/file.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c index 2229a74..1c0403a 100644 --- a/fs/ecryptfs/file.c +++ b/fs/ecryptfs/file.c @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct file *lower_file = NULL; long rc = -ENOTTY; - if (ecryptfs_file_to_private(file)) - lower_file = ecryptfs_file_to_lower(file); + if (!ecryptfs_file_to_private(file)) + goto out; + lower_file = ecryptfs_file_to_lower(file); if (lower_file-f_op-unlocked_ioctl) rc = lower_file-f_op-unlocked_ioctl(lower_file, cmd, arg); +out: return rc; } @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct file *lower_file = NULL; long rc = -ENOIOCTLCMD; - if (ecryptfs_file_to_private(file)) - lower_file = ecryptfs_file_to_lower(file); + if (!ecryptfs_file_to_private(file)) + goto out; + lower_file = ecryptfs_file_to_lower(file); if (lower_file-f_op lower_file-f_op-compat_ioctl) rc = lower_file-f_op-compat_ioctl(lower_file, cmd, arg); +out: return rc; } #endif -- 1.8.4.2 -- To unsubscribe from this list: send the line unsubscribe ecryptfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ecryptfs: Fix explicit null dereference
2013/11/14 Tyler Hicks tyhi...@canonical.com: On 2013-11-14 17:58:40, Geyslan Gregório Bem wrote: 2013/11/14 Tyler Hicks tyhi...@canonical.com: On 2013-11-14 15:42:14, Geyslan G. Bem wrote: If the condition 'ecryptfs_file_to_private(file)' takes false branch lower_file is dereferenced when NULL. Caught by Coverity: CIDs 1128834 and 1128833. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- Hello - Smatch picked up on this earlier in week and Dan analyzed the situation here: http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441 I agree with his assessment and proposed the following patch: http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442 It makes Smatch happy and it should also make Coverity happy. Tyler True. Disregard mine. Thanks Tyler. Thank you! Can I add your Reviewed-by: tag to that patch prior to pushing it to my next branch? Tyler Sure, Tyler. You can! :) Thanks again. fs/ecryptfs/file.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c index 2229a74..1c0403a 100644 --- a/fs/ecryptfs/file.c +++ b/fs/ecryptfs/file.c @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct file *lower_file = NULL; long rc = -ENOTTY; - if (ecryptfs_file_to_private(file)) - lower_file = ecryptfs_file_to_lower(file); + if (!ecryptfs_file_to_private(file)) + goto out; + lower_file = ecryptfs_file_to_lower(file); if (lower_file-f_op-unlocked_ioctl) rc = lower_file-f_op-unlocked_ioctl(lower_file, cmd, arg); +out: return rc; } @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct file *lower_file = NULL; long rc = -ENOIOCTLCMD; - if (ecryptfs_file_to_private(file)) - lower_file = ecryptfs_file_to_lower(file); + if (!ecryptfs_file_to_private(file)) + goto out; + lower_file = ecryptfs_file_to_lower(file); if (lower_file-f_op lower_file-f_op-compat_ioctl) rc = lower_file-f_op-compat_ioctl(lower_file, cmd, arg); +out: return rc; } #endif -- 1.8.4.2 -- To unsubscribe from this list: send the line unsubscribe ecryptfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe ecryptfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net/hsr: Fix possible leak in 'hsr_get_node_status()'
2013/11/14 Geyslan G. Bem geys...@gmail.com: If 'hsr_get_node_data()' returns error, going directly to 'fail' label doesn't free the memory pointed by 'skb_out'. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- net/hsr/hsr_netlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c index 4e66bf6..aef8429 100644 --- a/net/hsr/hsr_netlink.c +++ b/net/hsr/hsr_netlink.c @@ -249,7 +249,7 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info) hsr_node_if2_age, hsr_node_if2_seq); if (res 0) - goto fail; + goto nla_put_failure; res = nla_put(skb_out, HSR_A_NODE_ADDR, ETH_ALEN, nla_data(info-attrs[HSR_A_NODE_ADDR])); -- 1.8.4.2 It's a Coverity catch: CID 1128855. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128444 - Dereference after null check (FORWARD_NULL) in glock.c
2013/11/12 Steven Whitehouse :
> Hi,
>
> On Tue, 2013-11-12 at 12:53 -0200, Geyslan Gregório Bem wrote:
>> Hello,
>>
>> Coverity detected in 'fs/gfs2/glock.c' a possible dereference after
>> null check. Maybe a positive one.
>>
>> There is a initial check for possible 'gl' NULL. After that 'gl' is
>> dereferenced in the looping check by __lockref_is_dead().
>>
>> What do you think?
>>
> I've already been sent a patch for that, and its in the GFS2 -fixes
> tree. Thanks,
>
> Steve.
Steve, thanks.
I'll close it in Coverity.
>
>>
>> 1875static int gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
>> 1876{
>> 1877struct gfs2_glock *gl;
>> 1878
>> 1879do {
>> 1880gl = gi->gl;
>>
>> 1. Condition "gl", taking false branch
>>
>> 2. var_compare_op: Comparing "gl" to null implies that "gl" might be null.
>> 1881if (gl) {
>> 1882gi->gl = glock_hash_next(gl);
>> 1883gi->nhash++;
>> 1884} else {
>>
>> 3. Condition "gi->hash >= (32768 /* 1 << 15 */)", taking false branch
>> 1885if (gi->hash >= GFS2_GL_HASH_SIZE) {
>> 1886rcu_read_unlock();
>> 1887return 1;
>> 1888}
>> 1889gi->gl = glock_hash_chain(gi->hash);
>> 1890gi->nhash = 0;
>> 1891}
>>
>> 4. Condition "gi->gl == NULL", taking true branch
>>
>> 7. Condition "gi->gl == NULL", taking false branch
>> 1892while (gi->gl == NULL) {
>> 1893gi->hash++;
>>
>> 5. Condition "gi->hash >= (32768 /* 1 << 15 */)", taking false branch
>> 1894if (gi->hash >= GFS2_GL_HASH_SIZE) {
>> 1895rcu_read_unlock();
>> 1896return 1;
>> 1897}
>> 1898gi->gl = glock_hash_chain(gi->hash);
>> 1899gi->nhash = 0;
>>
>> 6. Jumping back to the beginning of the loop
>> 1900}
>> 1901/* Skip entries for other sb and dead entries */
>>
>> 8. Condition "gi->sdp != gi->gl->gl_sbd", taking false branch
>>
>> CID 1128444 (#1 of 1): Dereference after null check (FORWARD_NULL)9.
>> var_deref_model: Passing null pointer ">gl_lockref" to function
>> "__lockref_is_dead(struct lockref const *)", which dereferences
>> it.[show details]
>> 1902} while (gi->sdp != gi->gl->gl_sbd ||
>> __lockref_is_dead(>gl_lockref));
>> 1903
>> 1904return 0;
>> 1905}
>>
>
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] Coverity 1128444 - Dereference after null check (FORWARD_NULL) in glock.c
Hello,
Coverity detected in 'fs/gfs2/glock.c' a possible dereference after
null check. Maybe a positive one.
There is a initial check for possible 'gl' NULL. After that 'gl' is
dereferenced in the looping check by __lockref_is_dead().
What do you think?
1875static int gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
1876{
1877struct gfs2_glock *gl;
1878
1879do {
1880gl = gi->gl;
1. Condition "gl", taking false branch
2. var_compare_op: Comparing "gl" to null implies that "gl" might be null.
1881if (gl) {
1882gi->gl = glock_hash_next(gl);
1883gi->nhash++;
1884} else {
3. Condition "gi->hash >= (32768 /* 1 << 15 */)", taking false branch
1885if (gi->hash >= GFS2_GL_HASH_SIZE) {
1886rcu_read_unlock();
1887return 1;
1888}
1889gi->gl = glock_hash_chain(gi->hash);
1890gi->nhash = 0;
1891}
4. Condition "gi->gl == NULL", taking true branch
7. Condition "gi->gl == NULL", taking false branch
1892while (gi->gl == NULL) {
1893gi->hash++;
5. Condition "gi->hash >= (32768 /* 1 << 15 */)", taking false branch
1894if (gi->hash >= GFS2_GL_HASH_SIZE) {
1895rcu_read_unlock();
1896return 1;
1897}
1898gi->gl = glock_hash_chain(gi->hash);
1899gi->nhash = 0;
6. Jumping back to the beginning of the loop
1900}
1901/* Skip entries for other sb and dead entries */
8. Condition "gi->sdp != gi->gl->gl_sbd", taking false branch
CID 1128444 (#1 of 1): Dereference after null check (FORWARD_NULL)9.
var_deref_model: Passing null pointer ">gl_lockref" to function
"__lockref_is_dead(struct lockref const *)", which dereferences
it.[show details]
1902} while (gi->sdp != gi->gl->gl_sbd ||
__lockref_is_dead(>gl_lockref));
1903
1904return 0;
1905}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128445 - Reliance on integer endianness
2013/11/12 Peter Zijlstra : > On Tue, Nov 12, 2013 at 12:19:13PM -0200, Geyslan Gregório Bem wrote: > > Cc: kernel...@googlegroups.com > > Don't cross-post to lists that don't allow public posts, and very much > don't cross-post to lists that bounce in incomprehensible gibberish :/ > Ok. No more. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128445 - Reliance on integer endianness
2013/11/12 Måns Rullgård :
> Geyslan Gregório Bem writes:
>
>> Hi,
>>
>> Coverity detected in 'arch/x86/kernel/cpu/perf_event_intel_uncore.c' a
>> possible reliance on integer endianness. Is that a positive one?
>
> No, x86 is always little endian.
>
>> static u64 ivt_uncore_irp_read_counter(struct intel_uncore_box *box,
>> struct perf_event *event)
>> 1369{
>> 1370struct pci_dev *pdev = box->pci_dev;
>> 1371struct hw_perf_event *hwc = >hw;
>> 1372u64 count = 0;
>> 1373
>>
>> CID 1128445 (#1 of 1): Reliance on integer endianness
>> (INCOMPATIBLE_CAST)incompatible_cast: Pointer "" points to an
>> object whose effective type is "unsigned long long" (64 bits,
>> unsigned) but is dereferenced as a narrower "unsigned int" (32 bits,
>> unsigned). This may lead to unexpected results depending on machine
>> endianness.[show details]
>>
>> 1374pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc->idx],
>> (u32 *));
>> 1375pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc->idx]
>> + 4, (u32 *) + 1);
>> 1376
>> 1377return count;
>> 1378}
>
> This looks intentional and correct apart from possible strict aliasing
> issues.
>
> --
> Måns Rullgård
> m...@mansr.com
All right. Thanks.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] Coverity 1128445 - Reliance on integer endianness
Hi,
Coverity detected in 'arch/x86/kernel/cpu/perf_event_intel_uncore.c' a
possible reliance on integer endianness. Is that a positive one?
static u64 ivt_uncore_irp_read_counter(struct intel_uncore_box *box,
struct perf_event *event)
1369{
1370struct pci_dev *pdev = box->pci_dev;
1371struct hw_perf_event *hwc = >hw;
1372u64 count = 0;
1373
CID 1128445 (#1 of 1): Reliance on integer endianness
(INCOMPATIBLE_CAST)incompatible_cast: Pointer "" points to an
object whose effective type is "unsigned long long" (64 bits,
unsigned) but is dereferenced as a narrower "unsigned int" (32 bits,
unsigned). This may lead to unexpected results depending on machine
endianness.[show details]
1374pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc->idx],
(u32 *));
1375pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc->idx]
+ 4, (u32 *) + 1);
1376
1377return count;
1378}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC] Coverity 1128445 - Reliance on integer endianness
Hi,
Coverity detected in 'arch/x86/kernel/cpu/perf_event_intel_uncore.c' a
possible reliance on integer endianness. Is that a positive one?
static u64 ivt_uncore_irp_read_counter(struct intel_uncore_box *box,
struct perf_event *event)
1369{
1370struct pci_dev *pdev = box-pci_dev;
1371struct hw_perf_event *hwc = event-hw;
1372u64 count = 0;
1373
CID 1128445 (#1 of 1): Reliance on integer endianness
(INCOMPATIBLE_CAST)incompatible_cast: Pointer count points to an
object whose effective type is unsigned long long (64 bits,
unsigned) but is dereferenced as a narrower unsigned int (32 bits,
unsigned). This may lead to unexpected results depending on machine
endianness.[show details]
1374pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc-idx],
(u32 *)count);
1375pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc-idx]
+ 4, (u32 *)count + 1);
1376
1377return count;
1378}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128445 - Reliance on integer endianness
2013/11/12 Måns Rullgård m...@mansr.com:
Geyslan Gregório Bem geys...@gmail.com writes:
Hi,
Coverity detected in 'arch/x86/kernel/cpu/perf_event_intel_uncore.c' a
possible reliance on integer endianness. Is that a positive one?
No, x86 is always little endian.
static u64 ivt_uncore_irp_read_counter(struct intel_uncore_box *box,
struct perf_event *event)
1369{
1370struct pci_dev *pdev = box-pci_dev;
1371struct hw_perf_event *hwc = event-hw;
1372u64 count = 0;
1373
CID 1128445 (#1 of 1): Reliance on integer endianness
(INCOMPATIBLE_CAST)incompatible_cast: Pointer count points to an
object whose effective type is unsigned long long (64 bits,
unsigned) but is dereferenced as a narrower unsigned int (32 bits,
unsigned). This may lead to unexpected results depending on machine
endianness.[show details]
1374pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc-idx],
(u32 *)count);
1375pci_read_config_dword(pdev, ivt_uncore_irp_ctrs[hwc-idx]
+ 4, (u32 *)count + 1);
1376
1377return count;
1378}
This looks intentional and correct apart from possible strict aliasing
issues.
--
Måns Rullgård
m...@mansr.com
All right. Thanks.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128445 - Reliance on integer endianness
2013/11/12 Peter Zijlstra pet...@infradead.org: On Tue, Nov 12, 2013 at 12:19:13PM -0200, Geyslan Gregório Bem wrote: Cc: kernel...@googlegroups.com Don't cross-post to lists that don't allow public posts, and very much don't cross-post to lists that bounce in incomprehensible gibberish :/ Ok. No more. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC] Coverity 1128444 - Dereference after null check (FORWARD_NULL) in glock.c
Hello,
Coverity detected in 'fs/gfs2/glock.c' a possible dereference after
null check. Maybe a positive one.
There is a initial check for possible 'gl' NULL. After that 'gl' is
dereferenced in the looping check by __lockref_is_dead().
What do you think?
1875static int gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
1876{
1877struct gfs2_glock *gl;
1878
1879do {
1880gl = gi-gl;
1. Condition gl, taking false branch
2. var_compare_op: Comparing gl to null implies that gl might be null.
1881if (gl) {
1882gi-gl = glock_hash_next(gl);
1883gi-nhash++;
1884} else {
3. Condition gi-hash = (32768 /* 1 15 */), taking false branch
1885if (gi-hash = GFS2_GL_HASH_SIZE) {
1886rcu_read_unlock();
1887return 1;
1888}
1889gi-gl = glock_hash_chain(gi-hash);
1890gi-nhash = 0;
1891}
4. Condition gi-gl == NULL, taking true branch
7. Condition gi-gl == NULL, taking false branch
1892while (gi-gl == NULL) {
1893gi-hash++;
5. Condition gi-hash = (32768 /* 1 15 */), taking false branch
1894if (gi-hash = GFS2_GL_HASH_SIZE) {
1895rcu_read_unlock();
1896return 1;
1897}
1898gi-gl = glock_hash_chain(gi-hash);
1899gi-nhash = 0;
6. Jumping back to the beginning of the loop
1900}
1901/* Skip entries for other sb and dead entries */
8. Condition gi-sdp != gi-gl-gl_sbd, taking false branch
CID 1128444 (#1 of 1): Dereference after null check (FORWARD_NULL)9.
var_deref_model: Passing null pointer gl-gl_lockref to function
__lockref_is_dead(struct lockref const *), which dereferences
it.[show details]
1902} while (gi-sdp != gi-gl-gl_sbd ||
__lockref_is_dead(gl-gl_lockref));
1903
1904return 0;
1905}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Coverity 1128444 - Dereference after null check (FORWARD_NULL) in glock.c
2013/11/12 Steven Whitehouse swhit...@redhat.com:
Hi,
On Tue, 2013-11-12 at 12:53 -0200, Geyslan Gregório Bem wrote:
Hello,
Coverity detected in 'fs/gfs2/glock.c' a possible dereference after
null check. Maybe a positive one.
There is a initial check for possible 'gl' NULL. After that 'gl' is
dereferenced in the looping check by __lockref_is_dead().
What do you think?
I've already been sent a patch for that, and its in the GFS2 -fixes
tree. Thanks,
Steve.
Steve, thanks.
I'll close it in Coverity.
1875static int gfs2_glock_iter_next(struct gfs2_glock_iter *gi)
1876{
1877struct gfs2_glock *gl;
1878
1879do {
1880gl = gi-gl;
1. Condition gl, taking false branch
2. var_compare_op: Comparing gl to null implies that gl might be null.
1881if (gl) {
1882gi-gl = glock_hash_next(gl);
1883gi-nhash++;
1884} else {
3. Condition gi-hash = (32768 /* 1 15 */), taking false branch
1885if (gi-hash = GFS2_GL_HASH_SIZE) {
1886rcu_read_unlock();
1887return 1;
1888}
1889gi-gl = glock_hash_chain(gi-hash);
1890gi-nhash = 0;
1891}
4. Condition gi-gl == NULL, taking true branch
7. Condition gi-gl == NULL, taking false branch
1892while (gi-gl == NULL) {
1893gi-hash++;
5. Condition gi-hash = (32768 /* 1 15 */), taking false branch
1894if (gi-hash = GFS2_GL_HASH_SIZE) {
1895rcu_read_unlock();
1896return 1;
1897}
1898gi-gl = glock_hash_chain(gi-hash);
1899gi-nhash = 0;
6. Jumping back to the beginning of the loop
1900}
1901/* Skip entries for other sb and dead entries */
8. Condition gi-sdp != gi-gl-gl_sbd, taking false branch
CID 1128444 (#1 of 1): Dereference after null check (FORWARD_NULL)9.
var_deref_model: Passing null pointer gl-gl_lockref to function
__lockref_is_dead(struct lockref const *), which dereferences
it.[show details]
1902} while (gi-sdp != gi-gl-gl_sbd ||
__lockref_is_dead(gl-gl_lockref));
1903
1904return 0;
1905}
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] tracing: fix referencing after memory freeing and refactors code
2013/11/6 Steven Rostedt :
> On Wed, 6 Nov 2013 16:02:51 -0300
> "Geyslan G. Bem" wrote:
>
>> In 'system_tr_open()':
>> Fix possible 'dir' assignment after freeing it.
>
> I'll take this patch, but I'm going to reword the subject and the
> change log. The assignment of dir to filp->private_data after dir is
> freed is not as bad as it sounds. As we are returning an error,
> filp->private_data is never used.
>
> -- Steve
>
>
Ok Steve. Please, do the reword.
>>
>> In both functions:
>> Restructures logic conditions testing 'tracing_is_disabled()'
>> return before the others tests.
>>
>> Signed-off-by: Geyslan G. Bem
>> ---
>> kernel/trace/trace_events.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 368a4d5..b44a7ea 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1062,6 +1062,9 @@ static int subsystem_open(struct inode *inode, struct
>> file *filp)
>> struct trace_array *tr;
>> int ret;
>>
>> + if (tracing_is_disabled())
>> + return -ENODEV;
>> +
>> /* Make sure the system still exists */
>> mutex_lock(_types_lock);
>> mutex_lock(_mutex);
>> @@ -1108,6 +,9 @@ static int system_tr_open(struct inode *inode, struct
>> file *filp)
>> struct trace_array *tr = inode->i_private;
>> int ret;
>>
>> + if (tracing_is_disabled())
>> + return -ENODEV;
>> +
>> if (trace_array_get(tr) < 0)
>> return -ENODEV;
>>
>> @@ -1124,11 +1130,12 @@ static int system_tr_open(struct inode *inode,
>> struct file *filp)
>> if (ret < 0) {
>> trace_array_put(tr);
>> kfree(dir);
>> + return ret;
>> }
>>
>> filp->private_data = dir;
>>
>> - return ret;
>> + return 0;
>> }
>>
>> static int subsystem_release(struct inode *inode, struct file *file)
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code
2013/11/6 Steven Rostedt :
> Sorry for the late review, I was busy getting ready for kernel summit
> and then traveling too much.
>
No problem.
>
> On Fri, 18 Oct 2013 17:59:42 -0300
> "Geyslan G. Bem" wrote:
>
>> In 'system_tr_open()':
>> Fix possible 'dir' assignment after freeing it.
>>
>> In both functions:
>> Restructures logic conditions testing 'tracing_is_disabled()'
>> return before the others tests.
>> Centralizes the exiting in accordance to Coding Style, Chapter 7.
>>
>> Signed-off-by: Geyslan G. Bem
>> ---
>> kernel/trace/trace_events.c | 46
>> +++--
>> 1 file changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 368a4d5..0f56ebf 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct
>> file *filp)
>> struct event_subsystem *system = NULL;
>> struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
>> struct trace_array *tr;
>> - int ret;
>> + int ret = -ENODEV;
>> +
>> + if (tracing_is_disabled())
>> + return ret;
>>
>> /* Make sure the system still exists */
>> mutex_lock(_types_lock);
>> @@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode,
>> struct file *filp)
>> mutex_unlock(_types_lock);
>>
>> if (!system)
>> - return -ENODEV;
>> + return ret;
>>
>> /* Some versions of gcc think dir can be uninitialized here */
>> WARN_ON(!dir);
>>
>> /* Still need to increment the ref count of the system */
>> - if (trace_array_get(tr) < 0) {
>> - put_system(dir);
>> - return -ENODEV;
>> - }
>> + ret = trace_array_get(tr);
>> + if (ret)
>> + goto err_get;
>>
>> - ret = tracing_open_generic(inode, filp);
>> - if (ret < 0) {
>> - trace_array_put(tr);
>> - put_system(dir);
>> - }
>> + filp->private_data = dir;
>> + return 0;
>>
>> +err_get:
>> + put_system(dir);
>> return ret;
>
> I don't see any improvement in the above code, except for the initial
> check of tracing_is_disabled(). Just add:
>
> if (tracing_is_disabled())
> return -ENODEV;
>
> no need for all the fancy work with using ret and goto.
>
>
>> }
>>
>> @@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode,
>> struct file *filp)
>> {
>> struct ftrace_subsystem_dir *dir;
>> struct trace_array *tr = inode->i_private;
>> - int ret;
>> + int ret = -ENODEV;
>>
>> - if (trace_array_get(tr) < 0)
>> - return -ENODEV;
>> + if (tracing_is_disabled())
>> + return ret;
>> +
>> + ret = trace_array_get(tr);
>> + if (ret)
>> + return ret;
>>
>> /* Make a temporary dir that has no system but points to tr */
>> dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>> if (!dir) {
>> - trace_array_put(tr);
>> - return -ENOMEM;
>> + ret = -ENOMEM;
>> + goto err_dir;
>> }
>>
>> dir->tr = tr;
>>
>> - ret = tracing_open_generic(inode, filp);
>> - if (ret < 0) {
>> - trace_array_put(tr);
>> - kfree(dir);
>> - }
>> -
>> filp->private_data = dir;
>> + return 0;
>>
>
> Again, we don't need the goto and err_dir. The simple fix is to finish
> the function with:
>
> ret = tracing_open_generic(inode, filp);
> if (ret < 0) {
> trace_array_put(tr);
> kfree(dir);
> return ret;
> }
>
> filp->private_data = dir;
>
> return 0;
>
> -- Steve
>
>> +err_dir:
>> + trace_array_put(tr);
>> return ret;
>> }
>>
>
Done:
[PATCH v2] tracing: fix referencing after memory freeing and refactors code
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] tracing: fix referencing after memory freeing and refactors code
2013/11/6 Steven Rostedt rost...@goodmis.org:
Sorry for the late review, I was busy getting ready for kernel summit
and then traveling too much.
No problem.
On Fri, 18 Oct 2013 17:59:42 -0300
Geyslan G. Bem geys...@gmail.com wrote:
In 'system_tr_open()':
Fix possible 'dir' assignment after freeing it.
In both functions:
Restructures logic conditions testing 'tracing_is_disabled()'
return before the others tests.
Centralizes the exiting in accordance to Coding Style, Chapter 7.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
kernel/trace/trace_events.c | 46
+++--
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 368a4d5..0f56ebf 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1060,7 +1060,10 @@ static int subsystem_open(struct inode *inode, struct
file *filp)
struct event_subsystem *system = NULL;
struct ftrace_subsystem_dir *dir = NULL; /* Initialize for gcc */
struct trace_array *tr;
- int ret;
+ int ret = -ENODEV;
+
+ if (tracing_is_disabled())
+ return ret;
/* Make sure the system still exists */
mutex_lock(trace_types_lock);
@@ -1082,23 +1085,21 @@ static int subsystem_open(struct inode *inode,
struct file *filp)
mutex_unlock(trace_types_lock);
if (!system)
- return -ENODEV;
+ return ret;
/* Some versions of gcc think dir can be uninitialized here */
WARN_ON(!dir);
/* Still need to increment the ref count of the system */
- if (trace_array_get(tr) 0) {
- put_system(dir);
- return -ENODEV;
- }
+ ret = trace_array_get(tr);
+ if (ret)
+ goto err_get;
- ret = tracing_open_generic(inode, filp);
- if (ret 0) {
- trace_array_put(tr);
- put_system(dir);
- }
+ filp-private_data = dir;
+ return 0;
+err_get:
+ put_system(dir);
return ret;
I don't see any improvement in the above code, except for the initial
check of tracing_is_disabled(). Just add:
if (tracing_is_disabled())
return -ENODEV;
no need for all the fancy work with using ret and goto.
}
@@ -1106,28 +1107,29 @@ static int system_tr_open(struct inode *inode,
struct file *filp)
{
struct ftrace_subsystem_dir *dir;
struct trace_array *tr = inode-i_private;
- int ret;
+ int ret = -ENODEV;
- if (trace_array_get(tr) 0)
- return -ENODEV;
+ if (tracing_is_disabled())
+ return ret;
+
+ ret = trace_array_get(tr);
+ if (ret)
+ return ret;
/* Make a temporary dir that has no system but points to tr */
dir = kzalloc(sizeof(*dir), GFP_KERNEL);
if (!dir) {
- trace_array_put(tr);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_dir;
}
dir-tr = tr;
- ret = tracing_open_generic(inode, filp);
- if (ret 0) {
- trace_array_put(tr);
- kfree(dir);
- }
-
filp-private_data = dir;
+ return 0;
Again, we don't need the goto and err_dir. The simple fix is to finish
the function with:
ret = tracing_open_generic(inode, filp);
if (ret 0) {
trace_array_put(tr);
kfree(dir);
return ret;
}
filp-private_data = dir;
return 0;
-- Steve
+err_dir:
+ trace_array_put(tr);
return ret;
}
Done:
[PATCH v2] tracing: fix referencing after memory freeing and refactors code
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] tracing: fix referencing after memory freeing and refactors code
2013/11/6 Steven Rostedt rost...@goodmis.org:
On Wed, 6 Nov 2013 16:02:51 -0300
Geyslan G. Bem geys...@gmail.com wrote:
In 'system_tr_open()':
Fix possible 'dir' assignment after freeing it.
I'll take this patch, but I'm going to reword the subject and the
change log. The assignment of dir to filp-private_data after dir is
freed is not as bad as it sounds. As we are returning an error,
filp-private_data is never used.
-- Steve
Ok Steve. Please, do the reword.
In both functions:
Restructures logic conditions testing 'tracing_is_disabled()'
return before the others tests.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
kernel/trace/trace_events.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 368a4d5..b44a7ea 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1062,6 +1062,9 @@ static int subsystem_open(struct inode *inode, struct
file *filp)
struct trace_array *tr;
int ret;
+ if (tracing_is_disabled())
+ return -ENODEV;
+
/* Make sure the system still exists */
mutex_lock(trace_types_lock);
mutex_lock(event_mutex);
@@ -1108,6 +,9 @@ static int system_tr_open(struct inode *inode, struct
file *filp)
struct trace_array *tr = inode-i_private;
int ret;
+ if (tracing_is_disabled())
+ return -ENODEV;
+
if (trace_array_get(tr) 0)
return -ENODEV;
@@ -1124,11 +1130,12 @@ static int system_tr_open(struct inode *inode,
struct file *filp)
if (ret 0) {
trace_array_put(tr);
kfree(dir);
+ return ret;
}
filp-private_data = dir;
- return ret;
+ return 0;
}
static int subsystem_release(struct inode *inode, struct file *file)
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/31 Ben Myers : > Hey Geyslan, > > On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote: >> On 10/23/13 3:34 PM, Ben Myers wrote: >> >> > xfs: fix possible NULL dereference in xlog_verify_iclog >> > >> > In xlog_verify_iclog a debug check of the incore log buffers prints an >> > error if icptr is null and then goes on to dereference the pointer >> > regardless. Convert this to an assert so that the intention is clear. >> > This was reported by Coverty. >> > >> > Reported-by: Geyslan G. Bem >> > Signed-off-by: Ben Myers >> >> Reviewed-by: Eric Sandeen > > Applied this. Many thanks! ;) > > -Ben It was a pleasure. o/ -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/31 Ben Myers b...@sgi.com: Hey Geyslan, On Wed, Oct 30, 2013 at 03:08:12PM -0500, Eric Sandeen wrote: On 10/23/13 3:34 PM, Ben Myers wrote: xfs: fix possible NULL dereference in xlog_verify_iclog In xlog_verify_iclog a debug check of the incore log buffers prints an error if icptr is null and then goes on to dereference the pointer regardless. Convert this to an assert so that the intention is clear. This was reported by Coverty. Reported-by: Geyslan G. Bem geys...@gmail.com Signed-off-by: Ben Myers b...@sgi.com Reviewed-by: Eric Sandeen sand...@redhat.com Applied this. Many thanks! ;) -Ben It was a pleasure. o/ -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] efivarfs: 'efivarfs_file_write' function reorganization
2013/10/30 Matt Fleming : > On Wed, 30 Oct, at 10:44:16AM, Geyslan Gregório Bem wrote: >> Do you want that I undo that? I aggre that the variable use only >> reduces the line code. > > Yes please. > > -- > Matt Fleming, Intel Open Source Technology Center Done: [PATCH v2] efivarfs: 'efivarfs_file_write' function reorganization -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] efivarfs: 'efivarfs_file_write' function reorganization
2013/10/30 Matt Fleming : > On Mon, 14 Oct, at 03:37:17PM, Geyslan G. Bem wrote: >> This reorganization: >> >> Adds 'attrsize' variable to make the code cleaner and more >> understandable, replacing all 'sizeof(attributes)'. >> >> Removes 'bytes' prior assignment due this new approach. >> >> Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'. >> >> Signed-off-by: Geyslan G. Bem >> --- >> fs/efivarfs/file.c | 23 +-- >> 1 file changed, 9 insertions(+), 14 deletions(-) > > Hmm.. I'm not convinced this is much of an improvement. I think removing > 'sizeof(attributes)' actually makes the code harder to read. > > -- > Matt Fleming, Intel Open Source Technology Center Do you want that I undo that? I aggre that the variable use only reduces the line code. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] efivarfs: 'efivarfs_file_write' function reorganization
2013/10/30 Matt Fleming m...@console-pimps.org: On Mon, 14 Oct, at 03:37:17PM, Geyslan G. Bem wrote: This reorganization: Adds 'attrsize' variable to make the code cleaner and more understandable, replacing all 'sizeof(attributes)'. Removes 'bytes' prior assignment due this new approach. Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'. Signed-off-by: Geyslan G. Bem geys...@gmail.com --- fs/efivarfs/file.c | 23 +-- 1 file changed, 9 insertions(+), 14 deletions(-) Hmm.. I'm not convinced this is much of an improvement. I think removing 'sizeof(attributes)' actually makes the code harder to read. -- Matt Fleming, Intel Open Source Technology Center Do you want that I undo that? I aggre that the variable use only reduces the line code. -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] efivarfs: 'efivarfs_file_write' function reorganization
2013/10/30 Matt Fleming m...@console-pimps.org: On Wed, 30 Oct, at 10:44:16AM, Geyslan Gregório Bem wrote: Do you want that I undo that? I aggre that the variable use only reduces the line code. Yes please. -- Matt Fleming, Intel Open Source Technology Center Done: [PATCH v2] efivarfs: 'efivarfs_file_write' function reorganization -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/28 Geyslan Gregório Bem : > 2013/10/28 Geyslan Gregório Bem >> >> 2013/10/27 Eric Van Hensbergen >>> >>> Looks like the right approach. The one other optional thing I mentioned >>> was support for passing NULL for rdev and not trying to parse the device >>> info when rdev == NULL. Its a very slight optimization in the grand scheme >>> of things, but would seem to be cleaner for the folks calling the function >>> who don't touch rdev after the fact... >>> >>> -eric >>> >> Great. Let me do the changes this afternoon. >> >> > Hi Eric and all. > > You requested to avoid the parsing of device when rdev is NULL, all > right? But I'm afraid that that manner the res (return value) can be > returned wrong when the bit mode is a device. Well, I did some > changes. In this new approach, when rdev is NULL, the function only > doesn't make the device, but returns the res (umode_t) nicely. > > Tell me if this approach is correct. Do I have to modify something else? > > -- > Regards, > > Geyslan G. Bem > hackingbits.com Eric, I sent the new patch: [PATCH] 9p: code refactor in vfs_inode.c -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/28 Geyslan Gregório Bem geys...@gmail.com: 2013/10/28 Geyslan Gregório Bem geys...@gmail.com 2013/10/27 Eric Van Hensbergen eri...@gmail.com Looks like the right approach. The one other optional thing I mentioned was support for passing NULL for rdev and not trying to parse the device info when rdev == NULL. Its a very slight optimization in the grand scheme of things, but would seem to be cleaner for the folks calling the function who don't touch rdev after the fact... -eric Great. Let me do the changes this afternoon. Hi Eric and all. You requested to avoid the parsing of device when rdev is NULL, all right? But I'm afraid that that manner the res (return value) can be returned wrong when the bit mode is a device. Well, I did some changes. In this new approach, when rdev is NULL, the function only doesn't make the device, but returns the res (umode_t) nicely. Tell me if this approach is correct. Do I have to modify something else? -- Regards, Geyslan G. Bem hackingbits.com Eric, I sent the new patch: [PATCH] 9p: code refactor in vfs_inode.c -- Regards, Geyslan G. Bem hackingbits.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/28 Geyslan Gregório Bem
>
> 2013/10/27 Eric Van Hensbergen
>>
>> Looks like the right approach. The one other optional thing I mentioned was
>> support for passing NULL for rdev and not trying to parse the device info
>> when rdev == NULL. Its a very slight optimization in the grand scheme of
>> things, but would seem to be cleaner for the folks calling the function who
>> don't touch rdev after the fact...
>>
>> -eric
>>
> Great. Let me do the changes this afternoon.
>
>
Hi Eric and all.
You requested to avoid the parsing of device when rdev is NULL, all
right? But I'm afraid that that manner the res (return value) can be
returned wrong when the bit mode is a device. Well, I did some
changes. In this new approach, when rdev is NULL, the function only
doesn't make the device, but returns the res (umode_t) nicely.
Tell me if this approach is correct. Do I have to modify something else?
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..e3d56f1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;
static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
{
-int res;
+u32 res;
res = mode & 0777;
if (S_ISDIR(mode))
res |= P9_DMDIR;
@@ -125,10 +125,9 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
struct p9_wstat *stat, dev_t *rdev)
{
-int res;
+umode_t res;
u32 mode = stat->mode;
-*rdev = 0;
res = p9mode2perm(v9ses, stat);
if ((mode & P9_DMDIR) == P9_DMDIR)
@@ -144,10 +143,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
&& (v9ses->nodev == 0)) {
char type = 0, ext[32];
-int major = -1, minor = -1;
+u32 major = 0, minor = 0;
strlcpy(ext, stat->extension, sizeof(ext));
-sscanf(ext, "%c %u %u", , , );
+if (sscanf(ext, "%c %u %u", , , ) < 3) {
+p9_debug(P9_DEBUG_ERROR,
+ "It's necessary define type [%c], major [%u] and minor [%u]" \
+ "values when mode is P9_DMDEVICE\n", type, major, minor);
+goto err_dev;
+}
switch (type) {
case 'c':
res |= S_IFCHR;
@@ -158,11 +162,18 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
default:
p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
type, stat->extension);
-};
-*rdev = MKDEV(major, minor);
+goto err_dev;
+}
+/* Only make device if rdev pointer isn't NULL */
+if (rdev)
+*rdev = MKDEV(major, minor);
} else
res |= S_IFREG;
-
+goto ret;
+err_dev:
+if (rdev)
+rdev = ERR_PTR(-EIO);
+ret:
return res;
}
@@ -460,13 +471,12 @@ void v9fs_evict_inode(struct inode *inode)
static int v9fs_test_inode(struct inode *inode, void *data)
{
-int umode;
-dev_t rdev;
+umode_t umode;
struct v9fs_inode *v9inode = V9FS_I(inode);
struct p9_wstat *st = (struct p9_wstat *)data;
struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
-umode = p9mode2unixmode(v9ses, st, );
+umode = p9mode2unixmode(v9ses, st, NULL);
/* don't match inode of different type */
if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
return 0;
@@ -526,6 +536,10 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
*/
inode->i_ino = i_ino;
umode = p9mode2unixmode(v9ses, st, );
+if (IS_ERR_VALUE(rdev)) {
+retval = rdev;
+goto error;
+}
retval = v9fs_init_inode(v9ses, inode, umode, rdev);
if (retval)
goto error;
@@ -1461,8 +1475,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde
int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
{
-int umode;
-dev_t rdev;
+umode_t umode;
loff_t i_size;
struct p9_wstat *st;
struct v9fs_session_info *v9ses;
@@ -1474,7 +1487,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
/*
* Don't update inode if the file type is different
*/
-umode = p9mode2unixmode(v9ses, st, );
+umode = p9mode2unixmode(v9ses, st, NULL);
if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
goto out;
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/28 Geyslan Gregório Bem geys...@gmail.com
2013/10/27 Eric Van Hensbergen eri...@gmail.com
Looks like the right approach. The one other optional thing I mentioned was
support for passing NULL for rdev and not trying to parse the device info
when rdev == NULL. Its a very slight optimization in the grand scheme of
things, but would seem to be cleaner for the folks calling the function who
don't touch rdev after the fact...
-eric
Great. Let me do the changes this afternoon.
Hi Eric and all.
You requested to avoid the parsing of device when rdev is NULL, all
right? But I'm afraid that that manner the res (return value) can be
returned wrong when the bit mode is a device. Well, I did some
changes. In this new approach, when rdev is NULL, the function only
doesn't make the device, but returns the res (umode_t) nicely.
Tell me if this approach is correct. Do I have to modify something else?
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..e3d56f1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;
static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
{
-int res;
+u32 res;
res = mode 0777;
if (S_ISDIR(mode))
res |= P9_DMDIR;
@@ -125,10 +125,9 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
struct p9_wstat *stat, dev_t *rdev)
{
-int res;
+umode_t res;
u32 mode = stat-mode;
-*rdev = 0;
res = p9mode2perm(v9ses, stat);
if ((mode P9_DMDIR) == P9_DMDIR)
@@ -144,10 +143,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
else if ((mode P9_DMDEVICE) (v9fs_proto_dotu(v9ses))
(v9ses-nodev == 0)) {
char type = 0, ext[32];
-int major = -1, minor = -1;
+u32 major = 0, minor = 0;
strlcpy(ext, stat-extension, sizeof(ext));
-sscanf(ext, %c %u %u, type, major, minor);
+if (sscanf(ext, %c %u %u, type, major, minor) 3) {
+p9_debug(P9_DEBUG_ERROR,
+ It's necessary define type [%c], major [%u] and minor [%u] \
+ values when mode is P9_DMDEVICE\n, type, major, minor);
+goto err_dev;
+}
switch (type) {
case 'c':
res |= S_IFCHR;
@@ -158,11 +162,18 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
default:
p9_debug(P9_DEBUG_ERROR, Unknown special type %c %s\n,
type, stat-extension);
-};
-*rdev = MKDEV(major, minor);
+goto err_dev;
+}
+/* Only make device if rdev pointer isn't NULL */
+if (rdev)
+*rdev = MKDEV(major, minor);
} else
res |= S_IFREG;
-
+goto ret;
+err_dev:
+if (rdev)
+rdev = ERR_PTR(-EIO);
+ret:
return res;
}
@@ -460,13 +471,12 @@ void v9fs_evict_inode(struct inode *inode)
static int v9fs_test_inode(struct inode *inode, void *data)
{
-int umode;
-dev_t rdev;
+umode_t umode;
struct v9fs_inode *v9inode = V9FS_I(inode);
struct p9_wstat *st = (struct p9_wstat *)data;
struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
-umode = p9mode2unixmode(v9ses, st, rdev);
+umode = p9mode2unixmode(v9ses, st, NULL);
/* don't match inode of different type */
if ((inode-i_mode S_IFMT) != (umode S_IFMT))
return 0;
@@ -526,6 +536,10 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
*/
inode-i_ino = i_ino;
umode = p9mode2unixmode(v9ses, st, rdev);
+if (IS_ERR_VALUE(rdev)) {
+retval = rdev;
+goto error;
+}
retval = v9fs_init_inode(v9ses, inode, umode, rdev);
if (retval)
goto error;
@@ -1461,8 +1475,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde
int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
{
-int umode;
-dev_t rdev;
+umode_t umode;
loff_t i_size;
struct p9_wstat *st;
struct v9fs_session_info *v9ses;
@@ -1474,7 +1487,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
/*
* Don't update inode if the file type is different
*/
-umode = p9mode2unixmode(v9ses, st, rdev);
+umode = p9mode2unixmode(v9ses, st, NULL);
if ((inode-i_mode S_IFMT) != (umode S_IFMT))
goto out;
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/23 Ben Myers :
> Hey Geyslan,
>
> On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
>> - Regarding the "possible new patch" subject, I humbly pass the ball to you.
>>
>> Thank you for the attention.
>
> Thank you for the patch. I would really prefer to commit this showing
> authorship from you, rather than a Reported-by. Can I mark you down?
>
> Regards,
> Ben
>
Thank you, Ben. Sure, you can mark me.
> ---
>
> xfs: fix possible NULL dereference in xlog_verify_iclog
>
> In xlog_verify_iclog a debug check of the incore log buffers prints an
> error if icptr is null and then goes on to dereference the pointer
> regardless. Convert this to an assert so that the intention is clear.
> This was reported by Coverty.
>
> Reported-by: Geyslan G. Bem
> Signed-off-by: Ben Myers
> ---
> fs/xfs/xfs_log.c |8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> Index: b/fs/xfs/xfs_log.c
> ===
> --- a/fs/xfs/xfs_log.c 2013-10-23 14:52:47.875216875 -0500
> +++ b/fs/xfs/xfs_log.c 2013-10-23 14:53:53.775245830 -0500
> @@ -3714,11 +3714,9 @@ xlog_verify_iclog(
> /* check validity of iclog pointers */
> spin_lock(>l_icloglock);
> icptr = log->l_iclog;
> - for (i=0; i < log->l_iclog_bufs; i++) {
> - if (icptr == NULL)
> - xfs_emerg(log->l_mp, "%s: invalid ptr", __func__);
> - icptr = icptr->ic_next;
> - }
> + for (i=0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next)
> + ASSERT(icptr);
> +
> if (icptr != log->l_iclog)
> xfs_emerg(log->l_mp, "%s: corrupt iclog ring", __func__);
> spin_unlock(>l_icloglock);
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/22 Eric Sandeen :
> On 10/22/13 4:03 PM, Dave Chinner wrote:
>> On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote:
>>> On 10/22/13 3:39 PM, Dave Chinner wrote:
>>>> On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote:
>>>>> 2013/10/21 Dave Chinner :
>>>>>> On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote:
>>>>>>> On 10/21/13 6:56 PM, Dave Chinner wrote:
>>>>>>>> On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote:
>>>>>>
>>>>>> Yes, but to continue the Devil's Advocate argument, the purpose of
>>>>>> debug code isn't to enlightent the casual reader or drive-by
>>>>>> patchers - it's to make life easier for people who actually spend
>>>>>> time debugging the code. And the people who need the debug code
>>>>>> are expected to understand why an ASSERT is not necessary. :)
>>>>>>
>>>>> Dave, Eric and Ben,
>>>>>
>>>>> This was catched by coverity (CID 102348).
>>>>
>>>> You should have put that in the patch description.
>>>>
>>>> Now I understand why there's been a sudden surge of irrelevant one
>>>> line changes from random people that have never touched XFS before.
>>>>
>>>>
>>>>
>>>> Ok, lets churn the code just to shut the stupid checker up. This
>>>> doesn't fix a bug, it doesn't change behaviour, it just makes
>>>> coverity happy. Convert it to the for loop plus ASSERT I mentioned
>>>> in a previous message.
>>>
>>> You know, I respectfully disagree, but we might just have to agree
>>> to disagree. The code, as it stands, tests for a null ptr
>>> and then dereferences it. That's always going to raise some
>>> eyebrows, coverity or not, debug code or not, drive by or not.
>>
>>> So even for future developers, making the code more self-
>>> documenting about this behavior would be a plus, whether it's by
>>> comment, by explicit ASSERT(), or whatever. (I don't think
>>> that xfs_emerg() has quite enough context to make it obvious.)
>>
>> Sure, but if weren't for the fact that Coverity warned about it,
>> nobody other that us people who work on the XFS code day in, day out
>> would have even cared about it.
>>
>> That's kind of my point - again, as the Devil's Advocate - that
>> coverity is encouraging drive-by "fixes" by people who don't
>> actually understand any of the context, history and/or culture
>> surrounding the code being modified.
>
Dave, If Coverity had not caught this, I could have never sent a patch
to xfs in my entire life.
So, I need not to know the xfs culture, code or context to identify a
flagrant, intentional or not, code that seems a bug.
Open source world works this way, all can help, but only a few decide
to do it. And there many ways to do it; static analysis is only one.
> They shouldn't have to, the code (or comments therein) should
> make it obvious. ;) (in a perfect world...)
>
>> I have no problems with real bugs being fixed, but if we are
>> modifying code for no gain other than closing "coverity doesn't like
>> it" bugs, then we *should* be questioning whether the change is
>> really necessary.
>
> But let's give Geyslan the benefit of the doubt, and realize that
> Coverity does find real things, and even if it originated w/ a
> Coverity CID, when one sees:
>
> if (!a)
> printk("a thing\n")
>
> a = a->b = . . .
>
> it looks suspicious to pretty much anyone. I don't think Geyslan
> sent it to shut Coverity up, he sent it because it looked like
> a bug worth fixing (after Coverity spotted it).
>
Eric, you're right. In this particular case, I didn't send to shut Coverity up.
And yeah, it looked like a bug that worth to fixing.
> Let's not be too hard on him for trying; I appreciate it more
> than spelling fixes and whitespace cleanups. ;)
>
> I agree that some Coverity CIDs are false, and we shouldn't
> mangle code just to make it happy, but I just don't think that's
> what's going on here. Let's imagine Geyslan saw 10 other CIDs
> and elected not to send changes, because they didn't look like
> they needed fixing, but this one looked like a bona fide bug.
>
Yep.
>> Asking the question may not change the outcome, but we need to ask
>> and answer the question regardless.
>
>>> (We don't have to change code to shut up coverity; we can flag
>>> it in the da
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/22 Eric Sandeen sand...@sandeen.net: On 10/22/13 4:03 PM, Dave Chinner wrote: On Tue, Oct 22, 2013 at 03:49:01PM -0500, Eric Sandeen wrote: On 10/22/13 3:39 PM, Dave Chinner wrote: On Tue, Oct 22, 2013 at 08:12:51AM -0200, Geyslan Gregório Bem wrote: 2013/10/21 Dave Chinner da...@fromorbit.com: On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote: On 10/21/13 6:56 PM, Dave Chinner wrote: On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote: Yes, but to continue the Devil's Advocate argument, the purpose of debug code isn't to enlightent the casual reader or drive-by patchers - it's to make life easier for people who actually spend time debugging the code. And the people who need the debug code are expected to understand why an ASSERT is not necessary. :) Dave, Eric and Ben, This was catched by coverity (CID 102348). You should have put that in the patch description. Now I understand why there's been a sudden surge of irrelevant one line changes from random people that have never touched XFS before. sigh Ok, lets churn the code just to shut the stupid checker up. This doesn't fix a bug, it doesn't change behaviour, it just makes coverity happy. Convert it to the for loop plus ASSERT I mentioned in a previous message. You know, I respectfully disagree, but we might just have to agree to disagree. The code, as it stands, tests for a null ptr and then dereferences it. That's always going to raise some eyebrows, coverity or not, debug code or not, drive by or not. So even for future developers, making the code more self- documenting about this behavior would be a plus, whether it's by comment, by explicit ASSERT(), or whatever. (I don't think that xfs_emerg() has quite enough context to make it obvious.) Sure, but if weren't for the fact that Coverity warned about it, nobody other that us people who work on the XFS code day in, day out would have even cared about it. That's kind of my point - again, as the Devil's Advocate - that coverity is encouraging drive-by fixes by people who don't actually understand any of the context, history and/or culture surrounding the code being modified. Dave, If Coverity had not caught this, I could have never sent a patch to xfs in my entire life. So, I need not to know the xfs culture, code or context to identify a flagrant, intentional or not, code that seems a bug. Open source world works this way, all can help, but only a few decide to do it. And there many ways to do it; static analysis is only one. They shouldn't have to, the code (or comments therein) should make it obvious. ;) (in a perfect world...) I have no problems with real bugs being fixed, but if we are modifying code for no gain other than closing coverity doesn't like it bugs, then we *should* be questioning whether the change is really necessary. But let's give Geyslan the benefit of the doubt, and realize that Coverity does find real things, and even if it originated w/ a Coverity CID, when one sees: if (!a) printk(a thing\n) a = a-b = . . . it looks suspicious to pretty much anyone. I don't think Geyslan sent it to shut Coverity up, he sent it because it looked like a bug worth fixing (after Coverity spotted it). Eric, you're right. In this particular case, I didn't send to shut Coverity up. And yeah, it looked like a bug that worth to fixing. Let's not be too hard on him for trying; I appreciate it more than spelling fixes and whitespace cleanups. ;) I agree that some Coverity CIDs are false, and we shouldn't mangle code just to make it happy, but I just don't think that's what's going on here. Let's imagine Geyslan saw 10 other CIDs and elected not to send changes, because they didn't look like they needed fixing, but this one looked like a bona fide bug. Yep. Asking the question may not change the outcome, but we need to ask and answer the question regardless. (We don't have to change code to shut up coverity; we can flag it in the database and nobody else will see it.) Only if you are the first to see it and make an executive decision that it's not necessary to fix :/ Or, you find it, send a patch that seems reasonable, get massive pushback, (hopefully) flag it, and resolve never come back to xfs again. ;) Really, FWIW, the whole discussion to me is somewhat prolix. Let's see: - We have a Coverity spot (Dereference after NULL check) = BUG. - You identified that was intentional and isn't a bug. Ok? - Regarding the possible new patch subject, I humbly pass the ball to you. Thank you for the attention. Geyslan Regards. -Eric Cheers, Dave. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/23 Ben Myers b...@sgi.com:
Hey Geyslan,
On Wed, Oct 23, 2013 at 08:58:51AM -0200, Geyslan Gregório Bem wrote:
- Regarding the possible new patch subject, I humbly pass the ball to you.
Thank you for the attention.
Thank you for the patch. I would really prefer to commit this showing
authorship from you, rather than a Reported-by. Can I mark you down?
Regards,
Ben
Thank you, Ben. Sure, you can mark me.
---
xfs: fix possible NULL dereference in xlog_verify_iclog
In xlog_verify_iclog a debug check of the incore log buffers prints an
error if icptr is null and then goes on to dereference the pointer
regardless. Convert this to an assert so that the intention is clear.
This was reported by Coverty.
Reported-by: Geyslan G. Bem geys...@gmail.com
Signed-off-by: Ben Myers b...@sgi.com
---
fs/xfs/xfs_log.c |8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
Index: b/fs/xfs/xfs_log.c
===
--- a/fs/xfs/xfs_log.c 2013-10-23 14:52:47.875216875 -0500
+++ b/fs/xfs/xfs_log.c 2013-10-23 14:53:53.775245830 -0500
@@ -3714,11 +3714,9 @@ xlog_verify_iclog(
/* check validity of iclog pointers */
spin_lock(log-l_icloglock);
icptr = log-l_iclog;
- for (i=0; i log-l_iclog_bufs; i++) {
- if (icptr == NULL)
- xfs_emerg(log-l_mp, %s: invalid ptr, __func__);
- icptr = icptr-ic_next;
- }
+ for (i=0; i log-l_iclog_bufs; i++, icptr = icptr-ic_next)
+ ASSERT(icptr);
+
if (icptr != log-l_iclog)
xfs_emerg(log-l_mp, %s: corrupt iclog ring, __func__);
spin_unlock(log-l_icloglock);
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/21 Geyslan Gregório Bem :
> 2013/10/21 Geyslan Gregório Bem :
>> 2013/10/20 Eric Van Hensbergen :
>>> Please resubmit a clean patch which includes the check of sscanf for exactly
>>> the correct number of arguments and handles errors properly in other cases.
>>> That last bit may be a bit problematic since right now the only errors are
>>> prints and we seem to be otherwise silently failing. Of course, looks like
>>> nothing else is checking return values from that function for error. We
>>> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
>>> all the places which matter (looks like there are a few places where rdev
>>> just gets discarded -- might even be a good idea to not parse rdev unless we
>>> need to by passing NULL to p9mode2unixmode.
>>>
>>> All in all, its a corner case which is only likely with a broken server, but
>>> the full clean up would seem to be:
>>> a) switch to u32's
>>> b) pass NULL when rdev just gets discarded and don't bother parsing when
>>> it is
>>> c) check the sscanf return validity
>>> d) on error set ERR_PTR in rdev and check on return before proceeding
>>>
>>> That's a lot of cleanup, I'll add it to my work queue if you don't have time
>>> to rework your patch.
>>>
>>
>> Eric, I would like to try with your guidance.
>>
>>> For the other patches, anyone you didn't see a response from me on today is
>>> being pulled into my for-next queue. Thanks for the cleanups.
>>>
>>> -eric
>>
>> Thanks for accept them.
>>
>>>
>>>
>>>
>>>
>>> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem
>>> wrote:
>>>>
>>>> Joe,
>>>>
>>>> Nice, I'll wait their reply, there are other p9 patches that I have
>>>> already sent and am awaiting Eric's.
>>>>
>>>> Thank you again.
>>>>
>>>> Geyslan Gregório Bem
>>>> hackingbits.com
>>>>
>
> Let me know if I got your requests:
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 94de6d1..8a332d0 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -63,7 +63,7 @@ static const struct inode_operations
> v9fs_symlink_inode_operations;
>
> static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
> {
> -int res;
> +u32 res;
> res = mode & 0777;
> if (S_ISDIR(mode))
> res |= P9_DMDIR;
> @@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
> static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
> struct p9_wstat *stat, dev_t *rdev)
> {
> -int res;
> +umode_t res;
> u32 mode = stat->mode;
>
> *rdev = 0;
> @@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
> else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
> && (v9ses->nodev == 0)) {
> char type = 0, ext[32];
> -int major = -1, minor = -1;
> +u32 major = 0, minor = 0;
>
> strlcpy(ext, stat->extension, sizeof(ext));
> -sscanf(ext, "%c %u %u", , , );
> +if (sscanf(ext, "%c %u %u", , , ) < 3) {
> +p9_debug(P9_DEBUG_ERROR,
> + "It's necessary define type [%u], major [u%] and minor
> [u%]" \
> + "values when using P9_DMDEVICE", type, major, minor);
> +goto err_dev;
> +}
> switch (type) {
> case 'c':
> res |= S_IFCHR;
> @@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
> default:
> p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
> type, stat->extension);
> +goto err_dev;
> };
> *rdev = MKDEV(major, minor);
> } else
> res |= S_IFREG;
> +goto ret;
>
> +err_dev:
> +rdev = ERR_PTR(-EIO);
> +ret:
> return res;
> }
>
> @@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)
>
> static int v9fs_test_inode(struct inode *inode, void *data)
> {
> -int umode;
> +umode_t umode;
> dev_t rdev;
> struct v9fs_inode *v9inode = V9FS_I(inode);
> struct p9_wstat *st = (struct p9_wstat *)data;
> struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
>
> umode = p9mode2unixmode(v9ses, st, );
> +
&
Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow
2013/10/21 Dirk Brandewie : > > > On Monday, October 21, 2013, Rafael J. Wysocki wrote: >> >> On Monday, October 21, 2013 03:43:51 PM Dirk Brandewie wrote: >> > On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: >> > > On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: >> > >> On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: >> > >>> The expression 'pstate << 8' is evaluated using 32-bit arithmetic >> > >>> while >> > >>> 'val' expects an expression of type u64. >> > >>> >> > >>> Signed-off-by: Geyslan G. Bem >> > >> Acked-by: Dirk Brandewie >> > > >> > > Actually, isn't (pstate << 8) guaranteed not to overflow? >> > > >> > >> > Yes, I was assuming this was caught by a static checking tool. >> Yes, it was caught by Coverity, and I didn't debug the possibles pstate's. >> What was caught by the tool was the fact that 1UL << 32 might overflow on >> 32-bit, so using BIT(32) wasn't correct. This is the entire output: CID 1108110 (#1 of 1): Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)overflow_before_widen: Potentially overflowing expression "pstate << 8" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic before being used in a context which expects an expression of type "u64" (64 bits, unsigned). To avoid overflow, cast the left operand to "u64" before performing the left shift. >> >> > I didn't see a downside to giving the compilier complete information. >> >> Well, in that case the function's argument should be u64 rather than int. >> >> Either you know that it won't overflow, in which case the explicit type >> casting doesn't change anything, or you are not sure, in which case it's >> better to use u64 as the original type anyway in my opinion. > > > pstate << 8 can't overflow we can drop this I realized that are five calls to intel_pstate_set_pstate() /drivers/cpufreq/intel_pstate.c:410 /drivers/cpufreq/intel_pstate.c:417 /drivers/cpufreq/intel_pstate.c:432 /drivers/cpufreq/intel_pstate.c:514 /drivers/cpufreq/intel_pstate.c:566 I really don't know if the values that pstate assumes could cause overflow. And I really don't know if these values may change in the future. So I have assumed that the most careful is to cast, making the code error proof. But you know the code, thus know what is better. ;) Cheers. > >> >> Thanks! >> >> -- >> I speak only for myself. >> Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/21 Dave Chinner : > On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote: >> On 10/21/13 6:56 PM, Dave Chinner wrote: >> > On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote: >> >> Hey, >> >> >> >> On Mon, Oct 21, 2013 at 06:12:18PM -0500, Eric Sandeen wrote: >> >>> On 10/21/13 5:44 PM, Dave Chinner wrote: >> On Mon, Oct 21, 2013 at 03:58:23PM -0500, Eric Sandeen wrote: >> > On 10/21/13 1:32 PM, Geyslan G. Bem wrote: >> >> This patch puts a 'break' in the true branch, avoiding the >> >> 'icptr->ic_next' >> >> dereferencing. >> > >> > Reviewed-by: Eric Sandeen >> >> Actually, NACK. >> >>> >> >>> I felt that one coming ;) >> >>> >> > Hm, yeah - cmn_err(CE_PANIC, " " ); used to BUG_ON, but the newer >> > xfs_emerg() doesn't. >> > >> > Dave, was that intentional? >> >> Of course it was. ;) xfs_emerg() is only called from the debug code >> in xlog_verify_iclog(), xlog_verify_tail_lsn and assfail(). >> >> In the case of assfail(), it has it's own BUG() call, so it does >> everything just fine. >> >> In the case of xlog_verify_iclog() when icptr is NULL, it will >> panic immediately after the message is printed, just like the old >> code. i.e. this patch isn't fixing anything we need fixed. >> >>> >> >>> A BUG() is probably warranted, then. >> >> >> >> I tend to agree with Eric on this point. If we want to crash, I'd rather >> >> our >> >> intent be very clear, rather than just see a null ptr deref. ;) >> > >> > Sure. ASSERT() would be better and more consistent with the rest of >> > the code. i.e: >> > >> > for (i = 0; i < log->l_iclog_bufs; i++, icptr = icptr->ic_next) >> > ASSERT(icptr); >> > >> > >> > >> > However, I keep coming back to the fact that what we are checking is >> > that the list is correctly circular and that and adding an >> > ASSERT(icptr) to panic if a pointer chase finds a null pointer is >> > kinda redundant, especially as: >> > >> > - there's already 2 comments for the function indicating >> > that it is checking the validity of the pointers and that >> > they are circular >> > - we have repeatedly, over many years, justified the removal >> > of ASSERT(ptr) from code like: >> > >> > ASSERT(ptr); >> > foo = ptr->foo; >> > >> > as it is redundant because production code will always >> > panic the machine in that situation via the dereference. >> > ASSERT() is for documenting assumptions and constraints >> > that are not obvious from the code context. >> > >> > IOWs, in this case the presence or absence of the ASSERT inside >> > *debug-only code* doesn't add any addition value to debugging such >> > problems, nor does it add any value in terms of documentation >> > because it's clear from the comments in the debug code that it >> > should not be NULL to begin with. >> > >> > >> >> I guess what's left as unclear is why we would prefer to panic >> vs. handling the error, even if it's in debug code. The caller can >> handle errors, so blowing up here sure doesn't look intentional. > > If the iclog list is corrupt and not circular, then we will simply > panic the next time it is iterated. Lots of log code iterates the > iclog list, such as log IO completion, switching iclogs, log forces, > etc. > >> Maybe the answer is it's debug code >> and we want to drop to the debugger or generate a vmcore at that >> point, but that's just been demonstrated as quite unclear to the >> casual reader. :) > > Yes, but to continue the Devil's Advocate argument, the purpose of > debug code isn't to enlightent the casual reader or drive-by > patchers - it's to make life easier for people who actually spend > time debugging the code. And the people who need the debug code > are expected to understand why an ASSERT is not necessary. :) > Dave, Eric and Ben, This was catched by coverity (CID 102348). Well, I got it now that was intentional and changed it in Coverity Triage. But I must assume that it is not the standard debugging, then I suggest you put at least a comment explaining why it does dereference after NULL check. Cheers. > Cheers, > > Dave. > -- > Dave Chinner > da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xfs: fix possible NULL dereference
2013/10/21 Dave Chinner da...@fromorbit.com: On Mon, Oct 21, 2013 at 07:00:59PM -0500, Eric Sandeen wrote: On 10/21/13 6:56 PM, Dave Chinner wrote: On Mon, Oct 21, 2013 at 06:18:49PM -0500, Ben Myers wrote: Hey, On Mon, Oct 21, 2013 at 06:12:18PM -0500, Eric Sandeen wrote: On 10/21/13 5:44 PM, Dave Chinner wrote: On Mon, Oct 21, 2013 at 03:58:23PM -0500, Eric Sandeen wrote: On 10/21/13 1:32 PM, Geyslan G. Bem wrote: This patch puts a 'break' in the true branch, avoiding the 'icptr-ic_next' dereferencing. Reviewed-by: Eric Sandeen sand...@redhat.com Actually, NACK. I felt that one coming ;) Hm, yeah - cmn_err(CE_PANIC, ); used to BUG_ON, but the newer xfs_emerg() doesn't. Dave, was that intentional? Of course it was. ;) xfs_emerg() is only called from the debug code in xlog_verify_iclog(), xlog_verify_tail_lsn and assfail(). In the case of assfail(), it has it's own BUG() call, so it does everything just fine. In the case of xlog_verify_iclog() when icptr is NULL, it will panic immediately after the message is printed, just like the old code. i.e. this patch isn't fixing anything we need fixed. A BUG() is probably warranted, then. I tend to agree with Eric on this point. If we want to crash, I'd rather our intent be very clear, rather than just see a null ptr deref. ;) Sure. ASSERT() would be better and more consistent with the rest of the code. i.e: for (i = 0; i log-l_iclog_bufs; i++, icptr = icptr-ic_next) ASSERT(icptr); Devil's Advocate However, I keep coming back to the fact that what we are checking is that the list is correctly circular and that and adding an ASSERT(icptr) to panic if a pointer chase finds a null pointer is kinda redundant, especially as: - there's already 2 comments for the function indicating that it is checking the validity of the pointers and that they are circular - we have repeatedly, over many years, justified the removal of ASSERT(ptr) from code like: ASSERT(ptr); foo = ptr-foo; as it is redundant because production code will always panic the machine in that situation via the dereference. ASSERT() is for documenting assumptions and constraints that are not obvious from the code context. IOWs, in this case the presence or absence of the ASSERT inside *debug-only code* doesn't add any addition value to debugging such problems, nor does it add any value in terms of documentation because it's clear from the comments in the debug code that it should not be NULL to begin with. /Devil's Advocate I guess what's left as unclear is why we would prefer to panic vs. handling the error, even if it's in debug code. The caller can handle errors, so blowing up here sure doesn't look intentional. If the iclog list is corrupt and not circular, then we will simply panic the next time it is iterated. Lots of log code iterates the iclog list, such as log IO completion, switching iclogs, log forces, etc. Maybe the answer is it's debug code and we want to drop to the debugger or generate a vmcore at that point, but that's just been demonstrated as quite unclear to the casual reader. :) Yes, but to continue the Devil's Advocate argument, the purpose of debug code isn't to enlightent the casual reader or drive-by patchers - it's to make life easier for people who actually spend time debugging the code. And the people who need the debug code are expected to understand why an ASSERT is not necessary. :) Dave, Eric and Ben, This was catched by coverity (CID 102348). Well, I got it now that was intentional and changed it in Coverity Triage. But I must assume that it is not the standard debugging, then I suggest you put at least a comment explaining why it does dereference after NULL check. Cheers. Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow
2013/10/21 Dirk Brandewie dirk.brande...@gmail.com: On Monday, October 21, 2013, Rafael J. Wysocki wrote: On Monday, October 21, 2013 03:43:51 PM Dirk Brandewie wrote: On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: The expression 'pstate 8' is evaluated using 32-bit arithmetic while 'val' expects an expression of type u64. Signed-off-by: Geyslan G. Bem geys...@gmail.com Acked-by: Dirk Brandewie dirk.j.brande...@intel.com Actually, isn't (pstate 8) guaranteed not to overflow? Yes, I was assuming this was caught by a static checking tool. Yes, it was caught by Coverity, and I didn't debug the possibles pstate's. What was caught by the tool was the fact that 1UL 32 might overflow on 32-bit, so using BIT(32) wasn't correct. This is the entire output: CID 1108110 (#1 of 1): Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)overflow_before_widen: Potentially overflowing expression pstate 8 with type int (32 bits, signed) is evaluated using 32-bit arithmetic before being used in a context which expects an expression of type u64 (64 bits, unsigned). To avoid overflow, cast the left operand to u64 before performing the left shift. I didn't see a downside to giving the compilier complete information. Well, in that case the function's argument should be u64 rather than int. Either you know that it won't overflow, in which case the explicit type casting doesn't change anything, or you are not sure, in which case it's better to use u64 as the original type anyway in my opinion. pstate 8 can't overflow we can drop this I realized that are five calls to intel_pstate_set_pstate() /drivers/cpufreq/intel_pstate.c:410 /drivers/cpufreq/intel_pstate.c:417 /drivers/cpufreq/intel_pstate.c:432 /drivers/cpufreq/intel_pstate.c:514 /drivers/cpufreq/intel_pstate.c:566 I really don't know if the values that pstate assumes could cause overflow. And I really don't know if these values may change in the future. So I have assumed that the most careful is to cast, making the code error proof. But you know the code, thus know what is better. ;) Cheers. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/21 Geyslan Gregório Bem geys...@gmail.com:
2013/10/21 Geyslan Gregório Bem geys...@gmail.com:
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
Please resubmit a clean patch which includes the check of sscanf for exactly
the correct number of arguments and handles errors properly in other cases.
That last bit may be a bit problematic since right now the only errors are
prints and we seem to be otherwise silently failing. Of course, looks like
nothing else is checking return values from that function for error. We
could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
all the places which matter (looks like there are a few places where rdev
just gets discarded -- might even be a good idea to not parse rdev unless we
need to by passing NULL to p9mode2unixmode.
All in all, its a corner case which is only likely with a broken server, but
the full clean up would seem to be:
a) switch to u32's
b) pass NULL when rdev just gets discarded and don't bother parsing when
it is
c) check the sscanf return validity
d) on error set ERR_PTR in rdev and check on return before proceeding
That's a lot of cleanup, I'll add it to my work queue if you don't have time
to rework your patch.
Eric, I would like to try with your guidance.
For the other patches, anyone you didn't see a response from me on today is
being pulled into my for-next queue. Thanks for the cleanups.
-eric
Thanks for accept them.
On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem geys...@gmail.com
wrote:
Joe,
Nice, I'll wait their reply, there are other p9 patches that I have
already sent and am awaiting Eric's.
Thank you again.
Geyslan Gregório Bem
hackingbits.com
Let me know if I got your requests:
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..8a332d0 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;
static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
{
-int res;
+u32 res;
res = mode 0777;
if (S_ISDIR(mode))
res |= P9_DMDIR;
@@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
struct p9_wstat *stat, dev_t *rdev)
{
-int res;
+umode_t res;
u32 mode = stat-mode;
*rdev = 0;
@@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
else if ((mode P9_DMDEVICE) (v9fs_proto_dotu(v9ses))
(v9ses-nodev == 0)) {
char type = 0, ext[32];
-int major = -1, minor = -1;
+u32 major = 0, minor = 0;
strlcpy(ext, stat-extension, sizeof(ext));
-sscanf(ext, %c %u %u, type, major, minor);
+if (sscanf(ext, %c %u %u, type, major, minor) 3) {
+p9_debug(P9_DEBUG_ERROR,
+ It's necessary define type [%u], major [u%] and minor
[u%] \
+ values when using P9_DMDEVICE, type, major, minor);
+goto err_dev;
+}
switch (type) {
case 'c':
res |= S_IFCHR;
@@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
default:
p9_debug(P9_DEBUG_ERROR, Unknown special type %c %s\n,
type, stat-extension);
+goto err_dev;
};
*rdev = MKDEV(major, minor);
} else
res |= S_IFREG;
+goto ret;
+err_dev:
+rdev = ERR_PTR(-EIO);
+ret:
return res;
}
@@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)
static int v9fs_test_inode(struct inode *inode, void *data)
{
-int umode;
+umode_t umode;
dev_t rdev;
struct v9fs_inode *v9inode = V9FS_I(inode);
struct p9_wstat *st = (struct p9_wstat *)data;
struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
umode = p9mode2unixmode(v9ses, st, rdev);
+
+if (IS_ERR(rdev))
+return 0;
+
/* don't match inode of different type */
if ((inode-i_mode S_IFMT) != (umode S_IFMT))
return 0;
@@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block
*sb,
*/
inode-i_ino = i_ino;
umode = p9mode2unixmode(v9ses, st, rdev);
+if (IS_ERR(rdev)) {
+retval = rdev;
+goto error;
+}
+
retval = v9fs_init_inode(v9ses, inode, umode, rdev);
if (retval)
goto error;
@@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde
int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
{
-int umode;
+umode_t umode;
dev_t rdev;
loff_t i_size;
+int ret = 0;
struct p9_wstat *st;
struct v9fs_session_info *v9ses;
@@ -1475,6 +1495,11 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/21 Geyslan Gregório Bem :
> 2013/10/20 Eric Van Hensbergen :
>> Please resubmit a clean patch which includes the check of sscanf for exactly
>> the correct number of arguments and handles errors properly in other cases.
>> That last bit may be a bit problematic since right now the only errors are
>> prints and we seem to be otherwise silently failing. Of course, looks like
>> nothing else is checking return values from that function for error. We
>> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
>> all the places which matter (looks like there are a few places where rdev
>> just gets discarded -- might even be a good idea to not parse rdev unless we
>> need to by passing NULL to p9mode2unixmode.
>>
>> All in all, its a corner case which is only likely with a broken server, but
>> the full clean up would seem to be:
>> a) switch to u32's
>> b) pass NULL when rdev just gets discarded and don't bother parsing when
>> it is
>> c) check the sscanf return validity
>> d) on error set ERR_PTR in rdev and check on return before proceeding
>>
>> That's a lot of cleanup, I'll add it to my work queue if you don't have time
>> to rework your patch.
>>
>
> Eric, I would like to try with your guidance.
>
>> For the other patches, anyone you didn't see a response from me on today is
>> being pulled into my for-next queue. Thanks for the cleanups.
>>
>> -eric
>
> Thanks for accept them.
>
>>
>>
>>
>>
>> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem
>> wrote:
>>>
>>> Joe,
>>>
>>> Nice, I'll wait their reply, there are other p9 patches that I have
>>> already sent and am awaiting Eric's.
>>>
>>> Thank you again.
>>>
>>> Geyslan Gregório Bem
>>> hackingbits.com
>>>
Let me know if I got your requests:
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..8a332d0 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;
static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
{
-int res;
+u32 res;
res = mode & 0777;
if (S_ISDIR(mode))
res |= P9_DMDIR;
@@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
struct p9_wstat *stat, dev_t *rdev)
{
-int res;
+umode_t res;
u32 mode = stat->mode;
*rdev = 0;
@@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
&& (v9ses->nodev == 0)) {
char type = 0, ext[32];
-int major = -1, minor = -1;
+u32 major = 0, minor = 0;
strlcpy(ext, stat->extension, sizeof(ext));
-sscanf(ext, "%c %u %u", , , );
+if (sscanf(ext, "%c %u %u", , , ) < 3) {
+p9_debug(P9_DEBUG_ERROR,
+ "It's necessary define type [%u], major [u%] and minor [u%]" \
+ "values when using P9_DMDEVICE", type, major, minor);
+goto err_dev;
+}
switch (type) {
case 'c':
res |= S_IFCHR;
@@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
default:
p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
type, stat->extension);
+goto err_dev;
};
*rdev = MKDEV(major, minor);
} else
res |= S_IFREG;
+goto ret;
+err_dev:
+rdev = ERR_PTR(-EIO);
+ret:
return res;
}
@@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)
static int v9fs_test_inode(struct inode *inode, void *data)
{
-int umode;
+umode_t umode;
dev_t rdev;
struct v9fs_inode *v9inode = V9FS_I(inode);
struct p9_wstat *st = (struct p9_wstat *)data;
struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
umode = p9mode2unixmode(v9ses, st, );
+
+if (IS_ERR(rdev))
+return 0;
+
/* don't match inode of different type */
if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
return 0;
@@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
*/
inode->i_ino = i_ino;
umode = p9mode2unixmode(v9ses, st, );
+if (IS_ERR(rdev)) {
+retval = rdev;
+goto error;
+}
+
retval = v9fs_init_inode(v9ses, inode, umode, rdev);
if (retval)
goto error;
@@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t r
Re: [PATCH] 9p: fix return value in case in v9fs_fid_xattr_set()
2013/10/21 Geyslan G. Bem :
> In case of error in the p9_client_write, the function v9fs_fid_xattr_set
> should return its negative value, what was never being done.
>
> In case of success it only retuned 0. Now it returns the 'offset'
> variable (write_count total).
>
> Signed-off-by: Geyslan G. Bem
> ---
> fs/9p/xattr.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
> index 3c28cdf..04133a1 100644
> --- a/fs/9p/xattr.c
> +++ b/fs/9p/xattr.c
> @@ -138,8 +138,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char
> *name,
> if (retval < 0) {
> p9_debug(P9_DEBUG_VFS, "p9_client_xattrcreate failed %d\n",
> retval);
> - p9_client_clunk(fid);
> - return retval;
> + goto err;
> }
> msize = fid->clnt->msize;
> while (value_len) {
> @@ -152,12 +151,15 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char
> *name,
> if (write_count < 0) {
> /* error in xattr write */
> retval = write_count;
> - break;
> + goto err;
> }
> offset += write_count;
> value_len -= write_count;
> }
> - return p9_client_clunk(fid);
> + retval = offset;
> +err:
> + p9_client_clunk(fid);
> + return retval;
> }
>
> ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t
> buffer_size)
> --
> 1.8.4
>
Eric,
As you can see, I modified the success return value to the offset
(write_count sum), following the same principle of
v9fs_fid_xattr_get():
http://lxr.free-electrons.com/source/fs/9p/xattr.c#L65
What do you think?
Regards.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: fix return value in case of error in v9fs_fid_xattr_set
2013/10/21 Geyslan Gregório Bem :
> At first, thanks for reply.
>
> 2013/10/20 Eric Van Hensbergen :
>> On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem wrote:
>>>
>>> In case of error in the p9_client_write, the function v9fs_fid_xattr_set
>>> should return its negative value, what was never being done.
>>>
>>> Signed-off-by: Geyslan G. Bem
>>> ---
>>> fs/9p/xattr.c | 9 -
>>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
>>> index 3c28cdf..0788388 100644
>>> --- a/fs/9p/xattr.c
>>> +++ b/fs/9p/xattr.c
>>> @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const
>>> char *name,
>>> write_count = value_len;
>>> write_count = p9_client_write(fid, ((char *)value)+offset,
>>> NULL, offset, write_count);
>>> - if (write_count < 0) {
>>> - /* error in xattr write */
>>> - retval = write_count;
>>> - break;
>>> - }
>>> + /* error in xattr write */
>>> + if (write_count < 0)
>>> + return write_count;
>>> +
>>>
>>
>>
>> So, I'm convinced that there's a problem here, but I think the solution in
>> the patch is incomplete. Simply returning wouldn't clunk the fid. I think
>> the right approach is likely to keep the break, clunk and return an error if
>> either the p9_client_write or the p9_client_clunk fails.
>>
>> I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be
>> clunking the fid -- but considering it's cloned the fid in its function
>> body, it does seem like it shoudl also be cleaning up after itself.
>>
>
> Right. I'll centralize the exiting assuring that fid will be clunked
> in case of fails.
>
>> -eric
>>
>>
>> -eric
>>
New version sent:
[PATCH] 9p: fix return value in case in v9fs_fid_xattr_set()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: proper use of the 'name' variable
2013/10/21 Geyslan Gregório Bem :
> 2013/10/20 Eric Van Hensbergen :
>> I reckon we should probably just get rid of name if its not being used. I
>> doubt the indirection is going to hurt anything here.
>>
>> -eric
>>
>
> Eric, you're right. Once that there's not assignment to name, the
> cycles are the same.
> I'll get rid of name var.
>
New patch sent.
[PATCH] 9p: remove useless 'name' variable and assignment
>>
>>
>> On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem wrote:
>>>
>>> The 'name' variable was assigned but never used. Hence puts its
>>> assignment to the top and makes proper use of its value.
>>>
>>> Signed-off-by: Geyslan G. Bem
>>> ---
>>> fs/9p/vfs_inode_dotl.c | 8
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
>>> index 6803758..86b6944 100644
>>> --- a/fs/9p/vfs_inode_dotl.c
>>> +++ b/fs/9p/vfs_inode_dotl.c
>>> @@ -772,8 +772,10 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
>>> inode *dir,
>>> struct p9_fid *dfid, *oldfid;
>>> struct v9fs_session_info *v9ses;
>>>
>>> + name = (char *) dentry->d_name.name;
>>> +
>>> p9_debug(P9_DEBUG_VFS, "dir ino: %lu, old_name: %s, new_name:
>>> %s\n",
>>> -dir->i_ino, old_dentry->d_name.name,
>>> dentry->d_name.name);
>>> +dir->i_ino, old_dentry->d_name.name, name);
>>>
>>> v9ses = v9fs_inode2v9ses(dir);
>>> dir_dentry = dentry->d_parent;
>>> @@ -785,9 +787,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
>>> inode *dir,
>>> if (IS_ERR(oldfid))
>>> return PTR_ERR(oldfid);
>>>
>>> - name = (char *) dentry->d_name.name;
>>> -
>>> - err = p9_client_link(dfid, oldfid, (char *)dentry->d_name.name);
>>> + err = p9_client_link(dfid, oldfid, name);
>>>
>>> if (err < 0) {
>>> p9_debug(P9_DEBUG_VFS, "p9_client_link failed %d\n", err);
>>> --
>>> 1.8.4
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/20 Eric Van Hensbergen :
> Please resubmit a clean patch which includes the check of sscanf for exactly
> the correct number of arguments and handles errors properly in other cases.
> That last bit may be a bit problematic since right now the only errors are
> prints and we seem to be otherwise silently failing. Of course, looks like
> nothing else is checking return values from that function for error. We
> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
> all the places which matter (looks like there are a few places where rdev
> just gets discarded -- might even be a good idea to not parse rdev unless we
> need to by passing NULL to p9mode2unixmode.
>
> All in all, its a corner case which is only likely with a broken server, but
> the full clean up would seem to be:
> a) switch to u32's
> b) pass NULL when rdev just gets discarded and don't bother parsing when
> it is
> c) check the sscanf return validity
> d) on error set ERR_PTR in rdev and check on return before proceeding
>
> That's a lot of cleanup, I'll add it to my work queue if you don't have time
> to rework your patch.
>
Eric, I would like to try with your guidance.
> For the other patches, anyone you didn't see a response from me on today is
> being pulled into my for-next queue. Thanks for the cleanups.
>
> -eric
Thanks for accept them.
>
>
>
>
> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem
> wrote:
>>
>> Joe,
>>
>> Nice, I'll wait their reply, there are other p9 patches that I have
>> already sent and am awaiting Eric's.
>>
>> Thank you again.
>>
>> Geyslan Gregório Bem
>> hackingbits.com
>>
>>
>> 2013/10/7 Joe Perches :
>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>> >> Joe,
>> >>
>> >> Thank you for reply.
>> >>
>> >> What do you think about:
>> >>
>> >> strncpy(ext, stat->extension, sizeof(ext));
>> >> + if (sscanf(ext, "%c %u %u", , , ) <
>> >> 3) {
>> >> + p9_debug(P9_DEBUG_ERROR,
>> >> + "It's necessary define type, major
>> >> and minor values when using P9_DMDEVICE");
>> >> + return res;
>> >> + }
>> >> switch (type) {
>> >> case 'c':
>> >> res |= S_IFCHR;
>> >> break;
>> >> ...
>> >> *rdev = MKDEV(major, minor);
>> >
>> > I think the plan 9 folk should figure out what's right.
>> >
>> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: proper use of the 'name' variable
2013/10/20 Eric Van Hensbergen :
> I reckon we should probably just get rid of name if its not being used. I
> doubt the indirection is going to hurt anything here.
>
> -eric
>
Eric, you're right. Once that there's not assignment to name, the
cycles are the same.
I'll get rid of name var.
>
>
> On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem wrote:
>>
>> The 'name' variable was assigned but never used. Hence puts its
>> assignment to the top and makes proper use of its value.
>>
>> Signed-off-by: Geyslan G. Bem
>> ---
>> fs/9p/vfs_inode_dotl.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
>> index 6803758..86b6944 100644
>> --- a/fs/9p/vfs_inode_dotl.c
>> +++ b/fs/9p/vfs_inode_dotl.c
>> @@ -772,8 +772,10 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
>> inode *dir,
>> struct p9_fid *dfid, *oldfid;
>> struct v9fs_session_info *v9ses;
>>
>> + name = (char *) dentry->d_name.name;
>> +
>> p9_debug(P9_DEBUG_VFS, "dir ino: %lu, old_name: %s, new_name:
>> %s\n",
>> -dir->i_ino, old_dentry->d_name.name,
>> dentry->d_name.name);
>> +dir->i_ino, old_dentry->d_name.name, name);
>>
>> v9ses = v9fs_inode2v9ses(dir);
>> dir_dentry = dentry->d_parent;
>> @@ -785,9 +787,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
>> inode *dir,
>> if (IS_ERR(oldfid))
>> return PTR_ERR(oldfid);
>>
>> - name = (char *) dentry->d_name.name;
>> -
>> - err = p9_client_link(dfid, oldfid, (char *)dentry->d_name.name);
>> + err = p9_client_link(dfid, oldfid, name);
>>
>> if (err < 0) {
>> p9_debug(P9_DEBUG_VFS, "p9_client_link failed %d\n", err);
>> --
>> 1.8.4
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: fix return value in case of error in v9fs_fid_xattr_set
At first, thanks for reply.
2013/10/20 Eric Van Hensbergen :
> On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem wrote:
>>
>> In case of error in the p9_client_write, the function v9fs_fid_xattr_set
>> should return its negative value, what was never being done.
>>
>> Signed-off-by: Geyslan G. Bem
>> ---
>> fs/9p/xattr.c | 9 -
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
>> index 3c28cdf..0788388 100644
>> --- a/fs/9p/xattr.c
>> +++ b/fs/9p/xattr.c
>> @@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const
>> char *name,
>> write_count = value_len;
>> write_count = p9_client_write(fid, ((char *)value)+offset,
>> NULL, offset, write_count);
>> - if (write_count < 0) {
>> - /* error in xattr write */
>> - retval = write_count;
>> - break;
>> - }
>> + /* error in xattr write */
>> + if (write_count < 0)
>> + return write_count;
>> +
>>
>
>
> So, I'm convinced that there's a problem here, but I think the solution in
> the patch is incomplete. Simply returning wouldn't clunk the fid. I think
> the right approach is likely to keep the break, clunk and return an error if
> either the p9_client_write or the p9_client_clunk fails.
>
> I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be
> clunking the fid -- but considering it's cloned the fid in its function
> body, it does seem like it shoudl also be cleaning up after itself.
>
Right. I'll centralize the exiting assuring that fid will be clunked
in case of fails.
> -eric
>
>
> -eric
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: fix return value in case of error in v9fs_fid_xattr_set
At first, thanks for reply.
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem geys...@gmail.com wrote:
In case of error in the p9_client_write, the function v9fs_fid_xattr_set
should return its negative value, what was never being done.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
fs/9p/xattr.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index 3c28cdf..0788388 100644
--- a/fs/9p/xattr.c
+++ b/fs/9p/xattr.c
@@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const
char *name,
write_count = value_len;
write_count = p9_client_write(fid, ((char *)value)+offset,
NULL, offset, write_count);
- if (write_count 0) {
- /* error in xattr write */
- retval = write_count;
- break;
- }
+ /* error in xattr write */
+ if (write_count 0)
+ return write_count;
+
So, I'm convinced that there's a problem here, but I think the solution in
the patch is incomplete. Simply returning wouldn't clunk the fid. I think
the right approach is likely to keep the break, clunk and return an error if
either the p9_client_write or the p9_client_clunk fails.
I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be
clunking the fid -- but considering it's cloned the fid in its function
body, it does seem like it shoudl also be cleaning up after itself.
Right. I'll centralize the exiting assuring that fid will be clunked
in case of fails.
-eric
-eric
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: proper use of the 'name' variable
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
I reckon we should probably just get rid of name if its not being used. I
doubt the indirection is going to hurt anything here.
-eric
Eric, you're right. Once that there's not assignment to name, the
cycles are the same.
I'll get rid of name var.
On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem geys...@gmail.com wrote:
The 'name' variable was assigned but never used. Hence puts its
assignment to the top and makes proper use of its value.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
fs/9p/vfs_inode_dotl.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 6803758..86b6944 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -772,8 +772,10 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
inode *dir,
struct p9_fid *dfid, *oldfid;
struct v9fs_session_info *v9ses;
+ name = (char *) dentry-d_name.name;
+
p9_debug(P9_DEBUG_VFS, dir ino: %lu, old_name: %s, new_name:
%s\n,
-dir-i_ino, old_dentry-d_name.name,
dentry-d_name.name);
+dir-i_ino, old_dentry-d_name.name, name);
v9ses = v9fs_inode2v9ses(dir);
dir_dentry = dentry-d_parent;
@@ -785,9 +787,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
inode *dir,
if (IS_ERR(oldfid))
return PTR_ERR(oldfid);
- name = (char *) dentry-d_name.name;
-
- err = p9_client_link(dfid, oldfid, (char *)dentry-d_name.name);
+ err = p9_client_link(dfid, oldfid, name);
if (err 0) {
p9_debug(P9_DEBUG_VFS, p9_client_link failed %d\n, err);
--
1.8.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
Please resubmit a clean patch which includes the check of sscanf for exactly
the correct number of arguments and handles errors properly in other cases.
That last bit may be a bit problematic since right now the only errors are
prints and we seem to be otherwise silently failing. Of course, looks like
nothing else is checking return values from that function for error. We
could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
all the places which matter (looks like there are a few places where rdev
just gets discarded -- might even be a good idea to not parse rdev unless we
need to by passing NULL to p9mode2unixmode.
All in all, its a corner case which is only likely with a broken server, but
the full clean up would seem to be:
a) switch to u32's
b) pass NULL when rdev just gets discarded and don't bother parsing when
it is
c) check the sscanf return validity
d) on error set ERR_PTR in rdev and check on return before proceeding
That's a lot of cleanup, I'll add it to my work queue if you don't have time
to rework your patch.
Eric, I would like to try with your guidance.
For the other patches, anyone you didn't see a response from me on today is
being pulled into my for-next queue. Thanks for the cleanups.
-eric
Thanks for accept them.
On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem geys...@gmail.com
wrote:
Joe,
Nice, I'll wait their reply, there are other p9 patches that I have
already sent and am awaiting Eric's.
Thank you again.
Geyslan Gregório Bem
hackingbits.com
2013/10/7 Joe Perches j...@perches.com:
On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
Joe,
Thank you for reply.
What do you think about:
strncpy(ext, stat-extension, sizeof(ext));
+ if (sscanf(ext, %c %u %u, type, major, minor)
3) {
+ p9_debug(P9_DEBUG_ERROR,
+ It's necessary define type, major
and minor values when using P9_DMDEVICE);
+ return res;
+ }
switch (type) {
case 'c':
res |= S_IFCHR;
break;
...
*rdev = MKDEV(major, minor);
I think the plan 9 folk should figure out what's right.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: proper use of the 'name' variable
2013/10/21 Geyslan Gregório Bem geys...@gmail.com:
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
I reckon we should probably just get rid of name if its not being used. I
doubt the indirection is going to hurt anything here.
-eric
Eric, you're right. Once that there's not assignment to name, the
cycles are the same.
I'll get rid of name var.
New patch sent.
[PATCH] 9p: remove useless 'name' variable and assignment
On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem geys...@gmail.com wrote:
The 'name' variable was assigned but never used. Hence puts its
assignment to the top and makes proper use of its value.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
fs/9p/vfs_inode_dotl.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 6803758..86b6944 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -772,8 +772,10 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
inode *dir,
struct p9_fid *dfid, *oldfid;
struct v9fs_session_info *v9ses;
+ name = (char *) dentry-d_name.name;
+
p9_debug(P9_DEBUG_VFS, dir ino: %lu, old_name: %s, new_name:
%s\n,
-dir-i_ino, old_dentry-d_name.name,
dentry-d_name.name);
+dir-i_ino, old_dentry-d_name.name, name);
v9ses = v9fs_inode2v9ses(dir);
dir_dentry = dentry-d_parent;
@@ -785,9 +787,7 @@ v9fs_vfs_link_dotl(struct dentry *old_dentry, struct
inode *dir,
if (IS_ERR(oldfid))
return PTR_ERR(oldfid);
- name = (char *) dentry-d_name.name;
-
- err = p9_client_link(dfid, oldfid, (char *)dentry-d_name.name);
+ err = p9_client_link(dfid, oldfid, name);
if (err 0) {
p9_debug(P9_DEBUG_VFS, p9_client_link failed %d\n, err);
--
1.8.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] 9p: fix return value in case of error in v9fs_fid_xattr_set
2013/10/21 Geyslan Gregório Bem geys...@gmail.com:
At first, thanks for reply.
2013/10/20 Eric Van Hensbergen eri...@gmail.com:
On Sat, Sep 28, 2013 at 6:32 PM, Geyslan G. Bem geys...@gmail.com wrote:
In case of error in the p9_client_write, the function v9fs_fid_xattr_set
should return its negative value, what was never being done.
Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
fs/9p/xattr.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index 3c28cdf..0788388 100644
--- a/fs/9p/xattr.c
+++ b/fs/9p/xattr.c
@@ -149,11 +149,10 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const
char *name,
write_count = value_len;
write_count = p9_client_write(fid, ((char *)value)+offset,
NULL, offset, write_count);
- if (write_count 0) {
- /* error in xattr write */
- retval = write_count;
- break;
- }
+ /* error in xattr write */
+ if (write_count 0)
+ return write_count;
+
So, I'm convinced that there's a problem here, but I think the solution in
the patch is incomplete. Simply returning wouldn't clunk the fid. I think
the right approach is likely to keep the break, clunk and return an error if
either the p9_client_write or the p9_client_clunk fails.
I suppose you could make a claim that v9fs_fid_xattr_set shouldn't be
clunking the fid -- but considering it's cloned the fid in its function
body, it does seem like it shoudl also be cleaning up after itself.
Right. I'll centralize the exiting assuring that fid will be clunked
in case of fails.
-eric
-eric
New version sent:
[PATCH] 9p: fix return value in case in v9fs_fid_xattr_set()
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
