Re: [3/3] kprobes-netpktlog-268-rc3.patch

[Andi Kleen <ak@muc.de>]

Prasanna S Panchamukhi <prasanna@in.ibm.com> writes:
> +
> +static struct jprobe netpkt[] = {
> +	{
> +		{.addr = (kprobe_opcode_t *) netif_rx},
> +		.entry = (kprobe_opcode_t *) jnetif_rx

[...] Perhaps it would be better to resolve this using kallsyms. 
This way you could support the user passing a list of functions
they want to be traced. and you wouldn't need to add all these 
EXPORT_SYMBOLS.

> +
> +	printk("Filtering of network packets enabled...\n");

Nit - it doesn't filter anything.

> +
> +static inline void netfilter_ip(struct sk_buff *skb)
> +{
> +	struct ipt_log_info info;
> +	struct iphdr *iph;
> +	/* Log IP options */
> +	info.logflags = IPT_LOG_IPOPT;
> +
> +	/*
> +	 * Check if the protocol is IP before dumping the packet.
> +	 */
> +	if (skb->protocol == htons(ETH_P_IP) )
> +	{
> +		iph = (struct iphdr *) skb->data;
> +
> +		if ((src_ip == 0 && iph->daddr == tgt_ip)
> +			|| (tgt_ip == 0 && iph->saddr == src_ip)
> +			|| (iph->saddr == src_ip && iph->daddr == tgt_ip))

This looks a bit fragile. I don't think skb->data == iphdr is guaranteed
everywhere (I'm surprised it works for your dupack hook for example, Does it
really?). Better is to use skb->nh.iph, but that is also not true
everywhere. Better would be probably to let this be passed
by the individual hook function.

Also a Lindent run on the file couldn't hurt.

-Andi