| Message ID | 1671053900-14923-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | multipath config fixes | expand |
On Wed, 2022-12-14 at 15:38 -0600, Benjamin Marzinski wrote: > The first two patches are a cleanup and a fix for a memory leak in > the > config code. The third patch improves multipath's validation of the > strings it passes directly into the table, features, path_selector, > and > hardware_handler. These three strings all have argument counts, and > getting them wrong causes the kernel to parse the table incorrectly. > When this happens the table load fails, but the error messages from > the > kernel are often completely unhelpful. A bad argument count will > cause > the rest of the table to be parsed incorrectly, and the kernel might > not > hit an unworkable token till later in the parsing. Multipath now > makes > sure that the count matches the actual number of arguments that it is > passing. > > V2 Changes (based on suggestions from Martin Wilck): > 1/3: unrolled loop in validate_config_strvec() to explicitly check > the > possible quote strings > 3/3: spaces is now a "const char * const" and doesn't include '\n' Hm, my suggestion was wrong. It shouldn't be a pointer at all but an array: static const char spaces[] = " \f\r\t\v"; "static" makes sure it's only initialized once, and ends up in the .rodata section. In practice, there's no significant difference between either version. So, for the set: Reviewed-by: Martin Wilck <mwilck@suse.com> Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Dec 15, 2022 at 11:34:20AM +0000, Martin Wilck wrote: > On Wed, 2022-12-14 at 15:38 -0600, Benjamin Marzinski wrote: > > The first two patches are a cleanup and a fix for a memory leak in > > the > > config code. The third patch improves multipath's validation of the > > strings it passes directly into the table, features, path_selector, > > and > > hardware_handler. These three strings all have argument counts, and > > getting them wrong causes the kernel to parse the table incorrectly. > > When this happens the table load fails, but the error messages from > > the > > kernel are often completely unhelpful. A bad argument count will > > cause > > the rest of the table to be parsed incorrectly, and the kernel might > > not > > hit an unworkable token till later in the parsing. Multipath now > > makes > > sure that the count matches the actual number of arguments that it is > > passing. > > > > V2 Changes (based on suggestions from Martin Wilck): > > 1/3: unrolled loop in validate_config_strvec() to explicitly check > > the > > possible quote strings > > 3/3: spaces is now a "const char * const" and doesn't include '\n' > > Hm, my suggestion was wrong. It shouldn't be a pointer at all but an > array: > > static const char spaces[] = " \f\r\t\v"; > > "static" makes sure it's only initialized once, and ends > up in the .rodata section. Dumb question. If you explicitly make it an array, does that mean the compiler will always allocate separate memory for it, even if there is another identical const array? With multiple pointers to the same const string, the compiler will often just have one string in memory, which all the pointers refer to. Not sure if the same thing happens when they're defined as arrays. -Ben > > In practice, there's no significant difference between either version. > So, for the set: > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Dec 15, 2022 at 09:20:29AM -0600, Benjamin Marzinski wrote: > On Thu, Dec 15, 2022 at 11:34:20AM +0000, Martin Wilck wrote: > > On Wed, 2022-12-14 at 15:38 -0600, Benjamin Marzinski wrote: > > > The first two patches are a cleanup and a fix for a memory leak in > > > the > > > config code. The third patch improves multipath's validation of the > > > strings it passes directly into the table, features, path_selector, > > > and > > > hardware_handler. These three strings all have argument counts, and > > > getting them wrong causes the kernel to parse the table incorrectly. > > > When this happens the table load fails, but the error messages from > > > the > > > kernel are often completely unhelpful. A bad argument count will > > > cause > > > the rest of the table to be parsed incorrectly, and the kernel might > > > not > > > hit an unworkable token till later in the parsing. Multipath now > > > makes > > > sure that the count matches the actual number of arguments that it is > > > passing. > > > > > > V2 Changes (based on suggestions from Martin Wilck): > > > 1/3: unrolled loop in validate_config_strvec() to explicitly check > > > the > > > possible quote strings > > > 3/3: spaces is now a "const char * const" and doesn't include '\n' > > > > Hm, my suggestion was wrong. It shouldn't be a pointer at all but an > > array: > > > > static const char spaces[] = " \f\r\t\v"; > > > > "static" makes sure it's only initialized once, and ends > > up in the .rodata section. > > Dumb question. If you explicitly make it an array, does that mean the > compiler will always allocate separate memory for it, even if there is > another identical const array? With multiple pointers to the same const > string, the compiler will often just have one string in memory, which all > the pointers refer to. Not sure if the same thing happens when they're > defined as arrays. > > -Ben FWIW, I just checked with gcc version 11.3.1. Using: static const char * const ptr1 = "test"; static const char * const ptr2 = "test"; static const char arr1[] = "test"; static const char arr2[] = "test"; printf("pointers are %s\n", (ptr1 == ptr2) ? "equal" : "not equal"); printf("arrays are %s\n", (arr1 == arr2) ? "equal" : "not equal"); I get: pointers are equal arrays are not equal So depending on whether or not we use this multiple times, we can save ourselves.. a whopping 6 bytes. At any rate, now I know. -Ben > > > > > In practice, there's no significant difference between either version. > > So, for the set: > > > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > > > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, 2022-12-15 at 09:20 -0600, Benjamin Marzinski wrote: > On Thu, Dec 15, 2022 at 11:34:20AM +0000, Martin Wilck wrote: > > > > Hm, my suggestion was wrong. It shouldn't be a pointer at all but > > an > > array: > > > > static const char spaces[] = " \f\r\t\v"; > > > > "static" makes sure it's only initialized once, and ends > > up in the .rodata section. > > Dumb question. If you explicitly make it an array, does that mean the > compiler will always allocate separate memory for it, even if there > is > another identical const array? With multiple pointers to the same > const > string, the compiler will often just have one string in memory, which > all > the pointers refer to. Not sure if the same thing happens when > they're > defined as arrays. I am unsure about it, either. A quick experiment shows that the compiler does not merge multiple const char arrays with the same value into one memory location, while it does for string constants. (I suppose this works only for constants defined in the same source file?). Also, if you use a pointer (rather than an array), the compiler is smart enough not to allocate memory for the pointer variable. At least in simple situations, it's probably the best idea to just use the string constant without declaring a variable. So you could have written #define SPACES " \f\r\t\v" ... p += strspn(p, SPACES); This would benefit from gcc's constant merging. In the case at hand, the compiler output would be nearly the same between all 3 approaches. But this would change if you started using SPACES in some other function. What definitely should not be used is const char spaces[] = " \f\r\t\v"; (without "static") because it causes the array to be allocated on the stack and re-initialized at every function call. The moral: we shouldn't try to be smarter than the compiler (this refers to my review :-/) Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
The first two patches are a cleanup and a fix for a memory leak in the config code. The third patch improves multipath's validation of the strings it passes directly into the table, features, path_selector, and hardware_handler. These three strings all have argument counts, and getting them wrong causes the kernel to parse the table incorrectly. When this happens the table load fails, but the error messages from the kernel are often completely unhelpful. A bad argument count will cause the rest of the table to be parsed incorrectly, and the kernel might not hit an unworkable token till later in the parsing. Multipath now makes sure that the count matches the actual number of arguments that it is passing. V2 Changes (based on suggestions from Martin Wilck): 1/3: unrolled loop in validate_config_strvec() to explicitly check the possible quote strings 3/3: spaces is now a "const char * const" and doesn't include '\n' Benjamin Marzinski (3): libmpathutil: simplify set_value and validate_config_strvec libmultipath: don't leak memory on invalid strings libmutipath: validate the argument count of config strings libmpathutil/parser.c | 109 +++++++++++++++++++--------------------- libmultipath/dict.c | 112 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 154 insertions(+), 67 deletions(-)