Re: [PATCH] New match module: byte rate match

[Laszlo Valko <valko@linux.karinthy.hu>]

On Mon, Jan 20, 2003 at 03:58:19PM +0000, Chris Wilson wrote:
> Hi all,
> 
> NetServers.co.uk would like to submit a new module for your perusal and
> hopefully for inclusion in Netfilter. It's based on ipt_limit, but matches
> the number of bytes per second instead of the number of packets. This
> means that it can be used for basic quality-of-service implementation,
> thus:

Hi Chris!

Please consider fixing this structure:

struct ipt_bytelimit_info {
       /* Public parameters, supplied from user space */
       u_int32_t rate;       /* Number of bytes credited per second */
       u_int32_t credit_cap; /* Maximum credit which may accumulate */

       /* Used internally by the kernel */
       u_int32_t jiffy_rate; /* Number of bytes credited per jiffy */
       unsigned long prev;   /* Time of last match, in jiffies */
       u_int32_t credit;     /* Current balance */
};

Using unsigned long in there will make your code inoperable on 64bit
platforms with 32bit userspace. Please change it to something similar
to this one:

struct ipt_bytelimit_info {
       /* Public parameters, supplied from user space */
       u_int32_t rate;       /* Number of bytes credited per second */
       u_int32_t credit_cap; /* Maximum credit which may accumulate */ 

       /* Used internally by the kernel */
       u_int32_t jiffy_rate; /* Number of bytes credited per jiffy */
       u_int32_t credit;     /* Current balance */
       union {
             unsigned long prev;   /* Time of last match, in jiffies */
             u_int8_t padding[8];  /* Cover prev always */
       } u;
};
   
Moving credit field upwards one slot will make the structure more
compact on 64bit platforms (24 bytes instead of 28).

You might as well eliminate unsigned long totally, but I'm not sure
whether it's easy considering that jiffies is unsigned long.


Also this is incorrect as r->credit is not of type int,

               DEBUGP("ipt_bytelimit: accepted %d bytes, "
                       "remaining credit %d\n", packet_size, r->credit);

so you should use the appropriate format specifier (%lu for unsigned long).


I know ipt_limit (that your module is based on) is broken as well,
but it's not easy to fix it without breaking compatibility. On the other
hand, we should not replicating a broken interface...

Laszlo