| 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
| ||
|
Date: Tue, 3 Mar 2009 21:19:09 +0300
From: Evgeniy Polyakov <zbr@...emap.net>
To: Neil Horman <nhorman@...driver.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuznet@....inr.ac.ru,
pekkas@...core.fi, jmorris@...ei.org, yoshfuji@...ux-ipv6.org,
kaber@...sh.net
Subject: Re: [Patch 4/5] Network Drop Monitor: Adding drop monitor implementation & Netlink protocol
Hi Neil.
On Tue, Mar 03, 2009 at 12:04:35PM -0500, Neil Horman (nhorman@...driver.com) wrote:
> +typedef enum {
> + NET_DM_CFG_VERSION = 0,
> + NET_DM_CFG_ALERT_COUNT,
> + NET_DM_CFG_ALERT_DELAY,
> + NET_DM_CFG_MAX,
> +} config_type_t;
> +
> +struct net_dm_config_entry {
> + config_type_t type;
> + uint64_t data;
> +};
> +
Ugh, please add either some comments about its alignment or some padding
fields.
> +struct net_dm_config_msg {
> + size_t entries;
> + struct net_dm_config_entry options[0];
> +};
Isn't size_t have different size on different platforms?
> +/*
> + * Globals, our netlink socket pointer
> + * and the work handle that will send up
> + * netlink alerts
> + */
> +struct sock *dm_sock;
> +
> +struct per_cpu_dm_data {
> + struct work_struct dm_alert_work;
> + struct sk_buff *skb;
> + atomic_t dm_hit_count;
> + struct timer_list send_timer;
> +};
> +
> +DEFINE_PER_CPU(struct per_cpu_dm_data, dm_cpu_data);
> +
Static dm data?
> +static spinlock_t send_lock = SPIN_LOCK_UNLOCKED;
DEFINE_SPINLOCK
> +static int dm_hit_limit = 64;
> +static int dm_delay = 1;
> +
Configurable?
> +static void send_dm_alert(struct work_struct *unused)
> +{
> + size_t al, size;
> + struct net_dm_alert_msg *msg;
> + struct nlmsghdr *nlh;
> + struct sk_buff *skb, *nskb;
> + struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> +
Should it be plain get_cpu_var() or disabled preemption?
> + al = sizeof(struct nlmsghdr);
Hmm, this worries a bit, shouldn't it use NLMSG_LENGTH() and friends?
> + al += sizeof(struct net_dm_alert_msg);
> + al += dm_hit_limit * sizeof(struct net_dm_drop_point);
> +
> +
> + /*
> + * Grab the skb we're about to send
> + */
> + skb = data->skb;
> +
> + /*
> + * Replace it with a new one
> + */
> + nskb = alloc_skb(al, GFP_KERNEL);
> + nlh = (struct nlmsghdr *)nskb->data;
> + memset(nlh, 0, sizeof(struct nlmsghdr));
> + nlh->nlmsg_type = NET_DM_ALERT;
> + msg = NLMSG_DATA(nlh);
> + memset(msg, 0, al);
> + skb_put(nskb, sizeof(struct net_dm_alert_msg));
> + skb_put(nskb, sizeof(struct nlmsghdr));
> +
> + data->skb = nskb;
> +
> + /*
> + * Make sure to fix up the length field on the nlmsghdr
> + */
> + nlh = (struct nlmsghdr *)skb->data;
> + msg = NLMSG_DATA(nlh);
> +
> + size = sizeof(struct nlmsghdr) + sizeof (struct net_dm_alert_msg);
> + size += msg->entries * sizeof(struct net_dm_drop_point);
> + nlh->nlmsg_len = NLMSG_LENGTH(size);
> +
> + /*
> + * And adjust the skb if we need to
> + */
> + if (nlh->nlmsg_len > size)
> + skb_put(skb, (nlh->nlmsg_len-size));
> +
> + /*
> + * Ship it!
> + */
> + NETLINK_CB(skb).dst_group = NET_DM_GRP_ALERTS;
Additional space sneaked in.
> + spin_lock(&send_lock);
> + netlink_broadcast(dm_sock, skb, 0, NET_DM_GRP_ALERTS, 0);
Another 3. Also why do you use zero allocation instead of GFP_ATOMIC?
Also why do you use global lock here?
> + spin_unlock(&send_lock);
> +
> + /*
> + * Reset the per_cpu counter. This unlocks the trace point
> + * So that we can collect for subsequent drops
> + */
> + atomic_set(&data->dm_hit_count, dm_hit_limit);
> +
Trailing tab and unneded new line.
> +}
> +
> +static void sched_send_work(unsigned long unused)
> +{
> + struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> +
Please add some comments that it is called from the timer interrupt and
thus should not disable preemption around per-cpu variable.
> + schedule_work(&data->dm_alert_work);
> +}
> +
> +static void trace_kfree_skb_hit(struct sk_buff *skb, void *location)
> +{
> + struct net_dm_alert_msg *msg;
> + struct nlmsghdr *nlh;
> + int i;
> + struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data);
> +
> +
> + if (!atomic_add_unless(&data->dm_hit_count, -1, 0)) {
> + /*
> + * we're already at zero, discard this hit
> + */
> + goto out;
> + }
> +
> + nlh = (struct nlmsghdr *)data->skb->data;
> + msg = NLMSG_DATA(nlh);
> + for (i=0; i < msg->entries; i++) {
> + if (!memcmp(&location, msg->points[i].pc, sizeof(void *))) {
Looks like a netlink message to/from userspace contains pointer. If this
is the case, then it is not the way to go.
> + msg->points[i].count++;
> + goto out;
> + }
> + }
> +
> + /*
> + * We need to create a new entry
> + */
> + skb_put(data->skb, sizeof(struct net_dm_drop_point));
What if there is no space?
> + memcpy(msg->points[msg->entries].pc, &location, sizeof(void *));
Please change the structures so that they never contain variable size
members like pointers or longs.
> + msg->points[msg->entries].count = 1;
> + msg->entries++;
> +
> + if (!timer_pending(&data->send_timer)) {
> + data->send_timer.expires = jiffies + dm_delay * HZ;
> + add_timer_on(&data->send_timer, smp_processor_id());
> + }
> +
> +out:
> + return;
> +}
> +static void drpmon_rcv(struct sk_buff *skb)
> +{
> + int status, type, pid, flags, nlmsglen, skblen;
> + struct nlmsghdr *nlh;
> +
> + skblen = skb->len;
> + if (skblen < sizeof(*nlh))
> + return;
> +
> + nlh = nlmsg_hdr(skb);
> + nlmsglen = nlh->nlmsg_len;
> + if (nlmsglen < sizeof(*nlh) || skblen < nlmsglen)
> + return;
> +
> + pid = nlh->nlmsg_pid;
> + flags = nlh->nlmsg_flags;
> + type = nlh->nlmsg_type;
> +
> + if (pid != 0)
> + return;
> +
> + if (!(flags & NLM_F_REQUEST))
> + RCV_SKB_FAIL(-ECOMM);
> +
> +
Additional newline.
> + if (type <= NET_DM_BASE)
> + return;
> +
> + if (type >= NET_DM_MAX)
> + RCV_SKB_FAIL(-EINVAL);
> +
> +
And another one.
> + status = dropmon_handle_msg(skb, type,
> + nlmsglen - NLMSG_LENGTH(0));
> + if (status < 0)
> + RCV_SKB_FAIL(status);
> +
> + if (flags & NLM_F_ACK)
> + netlink_ack(skb, nlh, 0);
> + return;
> +}
> +
> +static int __init init_net_drop_monitor(void)
> +{
> + int cpu;
> + size_t al;
> + struct net_dm_alert_msg *msg;
> + struct nlmsghdr *nlh;
> + struct per_cpu_dm_data *data;
> + printk(KERN_INFO "Initalizing network drop monitor service\n");
> +
> + if (sizeof(void *) > 8) {
> + printk(KERN_ERR "Unable to store program counters on this arch, Drop monitor failed\n");
Why? :)
> + return -ENOSPC;
> + }
> +
> + dm_sock = netlink_kernel_create(&init_net, NETLINK_DRPMON, NET_DM_GRP_ALERTS,
> + drpmon_rcv, NULL, THIS_MODULE);
> +
> + if (dm_sock == NULL) {
> + printk(KERN_ERR "Could not create drop monitor socket\n");
> + return -ENOMEM;
> + }
> +
> + al = sizeof(struct nlmsghdr);
> + al += sizeof(struct net_dm_alert_msg);
> + al += dm_hit_limit * sizeof(struct net_dm_drop_point);
> +
Please use NLMSG helpers here.
> + for_each_present_cpu(cpu) {
> + data = &per_cpu(dm_cpu_data, cpu);
> + data->skb = alloc_skb(al, GFP_KERNEL);
> + skb_put(data->skb, sizeof(struct nlmsghdr));
> + skb_put(data->skb, sizeof(struct net_dm_alert_msg));
> + nlh = (struct nlmsghdr *)data->skb->data;
> + memset(nlh, 0, sizeof(struct nlmsghdr));
> + nlh->nlmsg_type = NET_DM_ALERT;
> + msg = NLMSG_DATA(nlh);
> + memset(msg, 0, al);
> + INIT_WORK(&data->dm_alert_work, send_dm_alert);
> + atomic_set(&data->dm_hit_count, dm_hit_limit);
> + init_timer(&data->send_timer);
> + data->send_timer.data = cpu;
> + data->send_timer.function = sched_send_work;
> + }
> +
> + return 0;
> +}
> +
> +subsys_initcall(init_net_drop_monitor);
> --
> 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
--
Evgeniy Polyakov
--
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