Re: [RFC] Question about potential problem in net/ipv4/route.c

[Eric Dumazet <dada1@cosmosbay.com>]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 11 Oct 2006 15:11:18 +0200
> 
>> Using memcmp(ptr1, ptr2, sizeof(SOMEFIELD)) is dangerous because 
>> sizeof(SOMEFIELD) can be larger than the underlying object, because of 
>> alignment constraints.
>>
>> In this case, sizeof(fl1->nl_u.ip4_u) is 16, while fl1->nl_u.ip4_u is :
>>
>>                 struct {
>>                         __u32                   daddr;
>>                         __u32                   saddr;
>>                         __u32                   fwmark;
>>                         __u8                    tos;
>>                         __u8                    scope;
>>                 } ip4_u;
>>
>> So 14 bytes are really initialized, and 2 padding bytes might contain random 
>> values...
> 
> We always explicitly initialize the flows, and even for local stack
> assignment based initialization, gcc zeros out the padding bytes
> always.  For non-stack-local cases we do explicit memset()'s over the
> entire object.  So I think while not perfect, we're very much safe
> here.
> 

Not on my gcc here (gcc version 3.4.4) : It wont zeros out the padding bytes

# cat bug.c
struct s1 {
     long d;
     char c;
};

void bar()
{
struct s1 s = { .d = 123, .c = 'a'};
foo(&s, sizeof(s));
}
# gcc -O2 -S bug.c
# more bug.s
.globl bar
         .type   bar, @function
bar:
.LFB2:
         subq    $24, %rsp
.LCFI0:
         movl    $16, %esi
         xorl    %eax, %eax
         movq    %rsp, %rdi
         movq    $123, (%rsp)
         movb    $97, 8(%rsp)
         call    foo
         addq    $24, %rsp
         ret

So 9(%rsp) -> 15(%rsp) are not initialized

Same on more recent gcc (4.1.1)

>> fast comparison, we should do some clever long/int XOR operations to avoid 
>> many test/branches, like the optim we did in compare_ether_addr())
>>
>> As shown in profiles, "repz cmpsb" is really a dog... (and its use of 
>> esi/edi/ecx registers a high pressure for the compiler/optimizer)
> 
> Yes, for the fast comparisons it is almost certainly worth it to do
> something saner in compare_keys().
> 
> But looking at this further, compare_keys() is only used in hotpath
> situations where we are optimizing for a miss, such as during hash
> insert.  The optimization therefore might be less justified as a
> result.

Well, on this machine I have these oprofile numbers :

<rt_intern_hash>: /* rt_intern_hash total: 993464  0.3619 */

  31751  0.0116 :ffffffff803e8d26:   repz cmpsb %es:(%rdi),%ds:(%rsi)
433438  0.1579 :ffffffff803e8d28:   jne    ffffffff803e8f80 <rt_intern_hash+0x310>

Eric