Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Mon, 27 Aug 2018, Al Viro wrote:
> On Sun, Aug 26, 2018 at 11:35:17PM -0400, Julia Lawall wrote:
>
> > * x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
>
> I can name several you've missed right off the top of my head -
> vmalloc, kvmalloc, kmem_cache_alloc, kmem_cache_zalloc, variants
> with _trace slapped on, and that is not to mention the things like
> get_free_page or
OK, maybe for a given type the set of functions would be smaller.
>
> void *my_k3wl_alloc(u64 n) // 'cause all artificial limits suck, that's why
> {
> lots and lots of home-grown stats collection
> some tracepoints thrown in just for fun
> return kmalloc(n);
> }
>
> (and no, I'm not implying that net/sched folks had done anything of that
> sort; I have seen that and worse in drivers, though)
>
> > The * at the beginning of the line means to highlight what you are looking
> > for, which is done by making a diff in which the highlighted line
> > appears to be removed.
>
> Umm... Does that cover return, BTW? Or something like
> T *barf;
> extern void foo(T *p);
> foo(kmalloc(sizeof(*barf)));
It only covers the pattern that is shown, ie an assignment. For this,
another pattern would be needed. It would be necessary to match first the
call that one is concerned with and then go find the function definition
or prototype to find the type of the associated parameter. It is possible
to count the offset of the kmalloc call in the argument list and then get
the type at the corresponding offset in the parameter list of the function
declaration or prototype.
>
>
> > The limitation is the ability to figure out the type of x. If it is a
> > local variable, Coccinelle should have no problem. If it is a structure
> > field, it may be necessary to provide command line arguments like
> >
> > --all-includes --include-headers-for-types
> >
> > --all-includes means to try to find all include files that are mentioned
> > in the .c file. The next stronger option is --recursive includes, which
> > means include what all of the mentioned files include as well,
> > recursively. This tends to cause a major performance hit, because a lot
> > of code is being parsed. --include-headers-for-types heals a bit with
> > that, as it only considers the header files when computing type
> > information, and now when applying the rules.
> >
> > With respect to ifdefs around variable declarations and structure field
> > declaration, in these cases Coccinelle considers that it cannot make the
> > ifdef have an if-like control flow, and so if considers the #ifdef, #else
> > and #endif to be comments. Thus it takes into account only the last type
> > provided for a given variable.
>
> [snip]
>
> What about several variants of structure definition? Because ifdefs around
> includes do occur in the wild...
Such ifdefs would be ignored completely. I suspect that only the last
definition of the structure would be taken into account.
julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Mon, 27 Aug 2018, Al Viro wrote:
> On Sun, Aug 26, 2018 at 11:35:17PM -0400, Julia Lawall wrote:
>
> > * x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
>
> I can name several you've missed right off the top of my head -
> vmalloc, kvmalloc, kmem_cache_alloc, kmem_cache_zalloc, variants
> with _trace slapped on, and that is not to mention the things like
> get_free_page or
OK, maybe for a given type the set of functions would be smaller.
>
> void *my_k3wl_alloc(u64 n) // 'cause all artificial limits suck, that's why
> {
> lots and lots of home-grown stats collection
> some tracepoints thrown in just for fun
> return kmalloc(n);
> }
>
> (and no, I'm not implying that net/sched folks had done anything of that
> sort; I have seen that and worse in drivers, though)
>
> > The * at the beginning of the line means to highlight what you are looking
> > for, which is done by making a diff in which the highlighted line
> > appears to be removed.
>
> Umm... Does that cover return, BTW? Or something like
> T *barf;
> extern void foo(T *p);
> foo(kmalloc(sizeof(*barf)));
It only covers the pattern that is shown, ie an assignment. For this,
another pattern would be needed. It would be necessary to match first the
call that one is concerned with and then go find the function definition
or prototype to find the type of the associated parameter. It is possible
to count the offset of the kmalloc call in the argument list and then get
the type at the corresponding offset in the parameter list of the function
declaration or prototype.
>
>
> > The limitation is the ability to figure out the type of x. If it is a
> > local variable, Coccinelle should have no problem. If it is a structure
> > field, it may be necessary to provide command line arguments like
> >
> > --all-includes --include-headers-for-types
> >
> > --all-includes means to try to find all include files that are mentioned
> > in the .c file. The next stronger option is --recursive includes, which
> > means include what all of the mentioned files include as well,
> > recursively. This tends to cause a major performance hit, because a lot
> > of code is being parsed. --include-headers-for-types heals a bit with
> > that, as it only considers the header files when computing type
> > information, and now when applying the rules.
> >
> > With respect to ifdefs around variable declarations and structure field
> > declaration, in these cases Coccinelle considers that it cannot make the
> > ifdef have an if-like control flow, and so if considers the #ifdef, #else
> > and #endif to be comments. Thus it takes into account only the last type
> > provided for a given variable.
>
> [snip]
>
> What about several variants of structure definition? Because ifdefs around
> includes do occur in the wild...
Such ifdefs would be ignored completely. I suspect that only the last
definition of the structure would be taken into account.
julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, Aug 26, 2018 at 11:35:17PM -0400, Julia Lawall wrote:
> * x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
I can name several you've missed right off the top of my head -
vmalloc, kvmalloc, kmem_cache_alloc, kmem_cache_zalloc, variants
with _trace slapped on, and that is not to mention the things like
get_free_page or
void *my_k3wl_alloc(u64 n) // 'cause all artificial limits suck, that's why
{
lots and lots of home-grown stats collection
some tracepoints thrown in just for fun
return kmalloc(n);
}
(and no, I'm not implying that net/sched folks had done anything of that
sort; I have seen that and worse in drivers, though)
> The * at the beginning of the line means to highlight what you are looking
> for, which is done by making a diff in which the highlighted line
> appears to be removed.
Umm... Does that cover return, BTW? Or something like
T *barf;
extern void foo(T *p);
foo(kmalloc(sizeof(*barf)));
> The limitation is the ability to figure out the type of x. If it is a
> local variable, Coccinelle should have no problem. If it is a structure
> field, it may be necessary to provide command line arguments like
>
> --all-includes --include-headers-for-types
>
> --all-includes means to try to find all include files that are mentioned
> in the .c file. The next stronger option is --recursive includes, which
> means include what all of the mentioned files include as well,
> recursively. This tends to cause a major performance hit, because a lot
> of code is being parsed. --include-headers-for-types heals a bit with
> that, as it only considers the header files when computing type
> information, and now when applying the rules.
>
> With respect to ifdefs around variable declarations and structure field
> declaration, in these cases Coccinelle considers that it cannot make the
> ifdef have an if-like control flow, and so if considers the #ifdef, #else
> and #endif to be comments. Thus it takes into account only the last type
> provided for a given variable.
[snip]
What about several variants of structure definition? Because ifdefs around
includes do occur in the wild...
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, Aug 26, 2018 at 11:35:17PM -0400, Julia Lawall wrote:
> * x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
I can name several you've missed right off the top of my head -
vmalloc, kvmalloc, kmem_cache_alloc, kmem_cache_zalloc, variants
with _trace slapped on, and that is not to mention the things like
get_free_page or
void *my_k3wl_alloc(u64 n) // 'cause all artificial limits suck, that's why
{
lots and lots of home-grown stats collection
some tracepoints thrown in just for fun
return kmalloc(n);
}
(and no, I'm not implying that net/sched folks had done anything of that
sort; I have seen that and worse in drivers, though)
> The * at the beginning of the line means to highlight what you are looking
> for, which is done by making a diff in which the highlighted line
> appears to be removed.
Umm... Does that cover return, BTW? Or something like
T *barf;
extern void foo(T *p);
foo(kmalloc(sizeof(*barf)));
> The limitation is the ability to figure out the type of x. If it is a
> local variable, Coccinelle should have no problem. If it is a structure
> field, it may be necessary to provide command line arguments like
>
> --all-includes --include-headers-for-types
>
> --all-includes means to try to find all include files that are mentioned
> in the .c file. The next stronger option is --recursive includes, which
> means include what all of the mentioned files include as well,
> recursively. This tends to cause a major performance hit, because a lot
> of code is being parsed. --include-headers-for-types heals a bit with
> that, as it only considers the header files when computing type
> information, and now when applying the rules.
>
> With respect to ifdefs around variable declarations and structure field
> declaration, in these cases Coccinelle considers that it cannot make the
> ifdef have an if-like control flow, and so if considers the #ifdef, #else
> and #endif to be comments. Thus it takes into account only the last type
> provided for a given variable.
[snip]
What about several variants of structure definition? Because ifdefs around
includes do occur in the wild...
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Mon, 27 Aug 2018, Al Viro wrote:
> On Sun, Aug 26, 2018 at 10:00:46PM -0400, Julia Lawall wrote:
> >
> >
> > On Sun, 26 Aug 2018, Al Viro wrote:
> >
> > > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote:
> > > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote:
> > > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote:
> > > > >
> > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc.
> > > > > > > arguments. typeof is even worse in that respect.
> > > > > >
> > > > > > True. Semantic searches via tools like coccinelle could help here
> > > > > > but those searches are quite a bit slower than straightforward
> > > > > > greps.
> > > > >
> > > > > Those searches are .config-sensitive as well, which can be much more
> > > > > unpleasant than being slow...
> > > >
> > > > Are they? Julia?
> > >
> > > They work pretty much on preprocessor output level; if something it
> > > ifdef'ed
> > > out on given config, it won't be seen...
> >
> > Coccinelle doesn't care what is ifdef'd out. It only misses the things it
> > can't parse. Very strange ifdefs could indeed cause that, but it should
> > be a minor problem.
>
> OK, but... what does it do when it sees two definitions of a structure
> in different branches of #if/#else/#endif? I think I'm confused about
> what it can and cannot do; to restate the original problem:
> * we need to find all places where instances of given type
> are created. Assume it never is a member of struct/union/array and
> no static or auto duration instances exist - everything is dynamically
> allocated somewhere.
>
> Can coccinelle do that and if it can, what are the limitations?
Looking at the thread, it seems that what you are asking for is something
like:
@@
struct tcf_proto *x;
@@
* x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
The * at the beginning of the line means to highlight what you are looking
for, which is done by making a diff in which the highlighted line
appears to be removed.
The limitation is the ability to figure out the type of x. If it is a
local variable, Coccinelle should have no problem. If it is a structure
field, it may be necessary to provide command line arguments like
--all-includes --include-headers-for-types
--all-includes means to try to find all include files that are mentioned
in the .c file. The next stronger option is --recursive includes, which
means include what all of the mentioned files include as well,
recursively. This tends to cause a major performance hit, because a lot
of code is being parsed. --include-headers-for-types heals a bit with
that, as it only considers the header files when computing type
information, and now when applying the rules.
With respect to ifdefs around variable declarations and structure field
declaration, in these cases Coccinelle considers that it cannot make the
ifdef have an if-like control flow, and so if considers the #ifdef, #else
and #endif to be comments. Thus it takes into account only the last type
provided for a given variable. For example, in the following:
int main() {
#ifdef X
int x;
#else
char xx;
#endif
return x;
}
int main2() {
#ifdef X
char x;
#else
int x;
#endif
return x;
}
x is considered to have type int in both returns. But if xx is replaced
by x in the definition of main, then x at the point of the return will
have type char.
Around a statement, such as x = kmalloc(...), it should be able to
consider that both branches of an ifdef are possible.
But there are no absolute guarantees. If there is any problem in parsing
a line of some function, the whole function will be ignores.
julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Mon, 27 Aug 2018, Al Viro wrote:
> On Sun, Aug 26, 2018 at 10:00:46PM -0400, Julia Lawall wrote:
> >
> >
> > On Sun, 26 Aug 2018, Al Viro wrote:
> >
> > > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote:
> > > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote:
> > > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote:
> > > > >
> > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc.
> > > > > > > arguments. typeof is even worse in that respect.
> > > > > >
> > > > > > True. Semantic searches via tools like coccinelle could help here
> > > > > > but those searches are quite a bit slower than straightforward
> > > > > > greps.
> > > > >
> > > > > Those searches are .config-sensitive as well, which can be much more
> > > > > unpleasant than being slow...
> > > >
> > > > Are they? Julia?
> > >
> > > They work pretty much on preprocessor output level; if something it
> > > ifdef'ed
> > > out on given config, it won't be seen...
> >
> > Coccinelle doesn't care what is ifdef'd out. It only misses the things it
> > can't parse. Very strange ifdefs could indeed cause that, but it should
> > be a minor problem.
>
> OK, but... what does it do when it sees two definitions of a structure
> in different branches of #if/#else/#endif? I think I'm confused about
> what it can and cannot do; to restate the original problem:
> * we need to find all places where instances of given type
> are created. Assume it never is a member of struct/union/array and
> no static or auto duration instances exist - everything is dynamically
> allocated somewhere.
>
> Can coccinelle do that and if it can, what are the limitations?
Looking at the thread, it seems that what you are asking for is something
like:
@@
struct tcf_proto *x;
@@
* x = \(kmalloc\|kzalloc\|devm_kmalloc\|devm_kzalloc\)(...)
The * at the beginning of the line means to highlight what you are looking
for, which is done by making a diff in which the highlighted line
appears to be removed.
The limitation is the ability to figure out the type of x. If it is a
local variable, Coccinelle should have no problem. If it is a structure
field, it may be necessary to provide command line arguments like
--all-includes --include-headers-for-types
--all-includes means to try to find all include files that are mentioned
in the .c file. The next stronger option is --recursive includes, which
means include what all of the mentioned files include as well,
recursively. This tends to cause a major performance hit, because a lot
of code is being parsed. --include-headers-for-types heals a bit with
that, as it only considers the header files when computing type
information, and now when applying the rules.
With respect to ifdefs around variable declarations and structure field
declaration, in these cases Coccinelle considers that it cannot make the
ifdef have an if-like control flow, and so if considers the #ifdef, #else
and #endif to be comments. Thus it takes into account only the last type
provided for a given variable. For example, in the following:
int main() {
#ifdef X
int x;
#else
char xx;
#endif
return x;
}
int main2() {
#ifdef X
char x;
#else
int x;
#endif
return x;
}
x is considered to have type int in both returns. But if xx is replaced
by x in the definition of main, then x at the point of the return will
have type char.
Around a statement, such as x = kmalloc(...), it should be able to
consider that both branches of an ifdef are possible.
But there are no absolute guarantees. If there is any problem in parsing
a line of some function, the whole function will be ignores.
julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, Aug 26, 2018 at 10:00:46PM -0400, Julia Lawall wrote: > > > On Sun, 26 Aug 2018, Al Viro wrote: > > > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote: > > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > > > arguments. typeof is even worse in that respect. > > > > > > > > > > True. Semantic searches via tools like coccinelle could help here > > > > > but those searches are quite a bit slower than straightforward greps. > > > > > > > > Those searches are .config-sensitive as well, which can be much more > > > > unpleasant than being slow... > > > > > > Are they? Julia? > > > > They work pretty much on preprocessor output level; if something it ifdef'ed > > out on given config, it won't be seen... > > Coccinelle doesn't care what is ifdef'd out. It only misses the things it > can't parse. Very strange ifdefs could indeed cause that, but it should > be a minor problem. OK, but... what does it do when it sees two definitions of a structure in different branches of #if/#else/#endif? I think I'm confused about what it can and cannot do; to restate the original problem: * we need to find all places where instances of given type are created. Assume it never is a member of struct/union/array and no static or auto duration instances exist - everything is dynamically allocated somewhere. Can coccinelle do that and if it can, what are the limitations?
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, Aug 26, 2018 at 10:00:46PM -0400, Julia Lawall wrote: > > > On Sun, 26 Aug 2018, Al Viro wrote: > > > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote: > > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > > > arguments. typeof is even worse in that respect. > > > > > > > > > > True. Semantic searches via tools like coccinelle could help here > > > > > but those searches are quite a bit slower than straightforward greps. > > > > > > > > Those searches are .config-sensitive as well, which can be much more > > > > unpleasant than being slow... > > > > > > Are they? Julia? > > > > They work pretty much on preprocessor output level; if something it ifdef'ed > > out on given config, it won't be seen... > > Coccinelle doesn't care what is ifdef'd out. It only misses the things it > can't parse. Very strange ifdefs could indeed cause that, but it should > be a minor problem. OK, but... what does it do when it sees two definitions of a structure in different branches of #if/#else/#endif? I think I'm confused about what it can and cannot do; to restate the original problem: * we need to find all places where instances of given type are created. Assume it never is a member of struct/union/array and no static or auto duration instances exist - everything is dynamically allocated somewhere. Can coccinelle do that and if it can, what are the limitations?
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, 26 Aug 2018, Al Viro wrote: > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote: > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > > arguments. typeof is even worse in that respect. > > > > > > > > True. Semantic searches via tools like coccinelle could help here > > > > but those searches are quite a bit slower than straightforward greps. > > > > > > Those searches are .config-sensitive as well, which can be much more > > > unpleasant than being slow... > > > > Are they? Julia? > > They work pretty much on preprocessor output level; if something it ifdef'ed > out on given config, it won't be seen... Coccinelle doesn't care what is ifdef'd out. It only misses the things it can't parse. Very strange ifdefs could indeed cause that, but it should be a minor problem. julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, 26 Aug 2018, Al Viro wrote: > On Sun, Aug 26, 2018 at 03:26:54PM -0700, Joe Perches wrote: > > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > > arguments. typeof is even worse in that respect. > > > > > > > > True. Semantic searches via tools like coccinelle could help here > > > > but those searches are quite a bit slower than straightforward greps. > > > > > > Those searches are .config-sensitive as well, which can be much more > > > unpleasant than being slow... > > > > Are they? Julia? > > They work pretty much on preprocessor output level; if something it ifdef'ed > out on given config, it won't be seen... Coccinelle doesn't care what is ifdef'd out. It only misses the things it can't parse. Very strange ifdefs could indeed cause that, but it should be a minor problem. julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, 26 Aug 2018, Joe Perches wrote: > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > arguments. typeof is even worse in that respect. > > > > > > True. Semantic searches via tools like coccinelle could help here > > > but those searches are quite a bit slower than straightforward greps. > > > > Those searches are .config-sensitive as well, which can be much more > > unpleasant than being slow... > > Are they? Julia? I don't completely understand the question. Coccinelle doens't know anything about the configuration. julia
Re: [PATCH] net: sched: Fix memory exposure from short TCA_U32_SEL
On Sun, 26 Aug 2018, Joe Perches wrote: > On Sun, 2018-08-26 at 22:24 +0100, Al Viro wrote: > > On Sun, Aug 26, 2018 at 11:57:57AM -0700, Joe Perches wrote: > > > > > > That, BTW, is why I hate the use of sizeof(*p) in kmalloc, etc. > > > > arguments. typeof is even worse in that respect. > > > > > > True. Semantic searches via tools like coccinelle could help here > > > but those searches are quite a bit slower than straightforward greps. > > > > Those searches are .config-sensitive as well, which can be much more > > unpleasant than being slow... > > Are they? Julia? I don't completely understand the question. Coccinelle doens't know anything about the configuration. julia
