| 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
| ||
|
Message-ID: <BANLkTi=AfvjVh7AZkQiABC8r=DrBYEGrug@mail.gmail.com>
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
Powered by blists - more mailing lists