netdev - Re: [af-packet 2/2] Enhance af-packet to provide (near zero)lossless packet capture functionality

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 8 Jun 2011 18:01:03 -0400
From:	chetan loke <loke.chetan@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net,
	eric.dumazet@...il.com, kaber@...sh.net, johann.baudy@...-log.net,
	Chetan Loke <lokec@....neu.edu>
Subject: Re: [af-packet 2/2] Enhance af-packet to provide (near zero)lossless
 packet capture functionality

On Wed, Jun 8, 2011 at 12:18 AM, Joe Perches <joe@...ches.com> wrote:
> On Tue, 2011-06-07 at 23:13 -0400, Chetan Loke wrote:
>> Signed-off-by: Chetan Loke <lokec@....neu.edu>
>
> just trivia:
>
>> ---
>>  net/packet/af_packet.c |  878 +++++++++++++++++++++++++++++++++++++++++++++---
> []
>> +/* kbdq - kernel block descriptor queue */
>> +struct kbdq_core {
>> +     struct pgv      *pkbdq;
>> +     unsigned int    hdrlen;
>> +     unsigned char   reset_pending_on_curr_blk;
>> +     unsigned char   delete_blk_timer;
>> +     unsigned short  kactive_blk_num;
>> +     unsigned short  hole_bytes_size;
>> +     char            *pkblk_start;
>> +     char            *pkblk_end;
>> +     int             kblk_size;
>> +     unsigned int    knum_blocks;
>> +     uint64_t        knxt_seq_num;
>> +     char            *prev;
>> +     char            *nxt_offset;
>> +
>> +     /* last_kactive_blk_num:
>> +      * trick to see if user-space has caught up
>> +      * in order to avoid refreshing timer when every single pkt arrives.
>> +      */
>> +     unsigned short  last_kactive_blk_num;
>> +
>> +     atomic_t        blk_fill_in_prog;
>> +
>> +     /* Default is set to 8ms */
>> +#define DEFAULT_PRB_RETIRE_TOV       (8)
>> +
>> +     unsigned short  retire_blk_tov;
>> +     unsigned long   tov_in_jiffies;
>> +
>> +     /* timer to retire an outstanding block */
>> +     struct timer_list retire_blk_timer;
>> +};
>
> You could align the member entries a bit more,
> maybe move last_kactive_blk_num after retire_blk_tov
>
> []
>
>> @@ -248,8 +322,11 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
>>               h.h2->tp_status = status;
>>               flush_dcache_page(pgv_to_page(&h.h2->tp_status));
>>               break;
>> +     case TPACKET_V3:
>>       default:
>> -             pr_err("TPACKET version not supported\n");
>> +             pr_err("<%s> TPACKET version not supported.Who is calling?\
>> +                     Dumping stack.\n", __func__);
>
> whitespace defect because of line continuation.  Maybe just:
>                WARN(1, "TPACKET version not supported\n");
>
>>               BUG();
>>       }
>>
>> @@ -274,8 +351,11 @@ static int __packet_get_status(struct packet_sock *po, void *frame)
>>       case TPACKET_V2:
>>               flush_dcache_page(pgv_to_page(&h.h2->tp_status));
>>               return h.h2->tp_status;
>> +     case TPACKET_V3:
>>       default:
>> -             pr_err("TPACKET version not supported\n");
>> +             pr_err("<%s> TPACKET version:%d not supported.\
>> +                     Dumping stack.\n", __func__, po->tp_version);
>> +             dump_stack();
>
> here too.
>
>                WARN(1, "TPACKET version %d not supported\n", po->tp_version);
>
>>               BUG();
>>               return 0;
>>       }
> []
>> +static void prb_open_block(struct kbdq_core *pkc1, struct block_desc *pbd1)
>> +{
>> +     pr_err("<%s> ERROR block:%p is NOT FREE status:%d\
>> +                     kactive_blk_num:%d\n",
>> +                     __func__, pbd1, BLOCK_STATUS(pbd1), pkc1->kactive_blk_num);
>> +     dump_stack();
>> +     BUG();
>
> here too.  maybe just:
>        WARN(1, "%s: ERROR block:%p is not free.  status: %s kactive_blk_num:%d\n"
>             __func__, pbd1, BLOCK_STATUS(pbd1), pkc1->kactive_blk_num);
>
>> +static inline void packet_increment_rx_head(struct packet_sock *po,
>> +                                         struct packet_ring_buffer *rb)
>> +{
>> +     switch (po->tp_version) {
>> +     case TPACKET_V1:
>> +     case TPACKET_V2:
>> +             return packet_increment_head(rb);
>> +     case TPACKET_V3:
>> +     default:
>> +             pr_err("<%s> TPACKET version:%d not supported.\
>> +                     Dumping stack.\n", __func__, po->tp_version);
>
> whitespace, WARN(1, etc...
>
>
>> @@ -2412,7 +3168,7 @@ out_free_pgvec:
>>       goto out;
>>  }
>>
>> -static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
>> +static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>>               int closing, int tx_ring)
>>  {
>>       struct pgv *pg_vec = NULL;
>> @@ -2421,7 +3177,17 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
>>       struct packet_ring_buffer *rb;
>>       struct sk_buff_head *rb_queue;
>>       __be16 num;
>> -     int err;
>> +     int err = -EINVAL;
>> +     /* Added to avoid minimal code churn */
>> +     struct tpacket_req *req = &req_u->req;
>> +
>> +     /* Opening a Tx-ring is NOT supported in TPACKET_V3 */
>> +     if (!closing && tx_ring && (po->tp_version > TPACKET_V2)) {
>> +             pr_err("<%s> Tx-ring is not supported on version:%d.\
>> +                        Dumping stack.\n", __func__, po->tp_version);
>
> whitespace, WARN(1, etc...
>
>
>

Sure, will address all the comments(sob,alignment,pr_err etc) in the
next iteration of the patch.

thanks
Chetan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html