Re: connector.c

[Evgeniy Polyakov]

On Thu, 2005-03-31 at 17:30 -0800, Andrew Morton wrote:
> Some belated comments...
> 
> 
> > 
> > module_param(unit, int, 0);
> > module_param(cn_idx, uint, 0);
> > module_param(cn_val, uint, 0);
> 
> MODULE_PARM_DESC needed, please.

Yep.

> > static DEFINE_SPINLOCK(notify_lock);
> > static LIST_HEAD(notify_list);
> > 
> > static struct cn_dev cdev;
> > 
> > int cn_already_initialized = 0;
> > 
> > /*
> >  * msg->seq and msg->ack are used to determine message genealogy.
> >  * When someone sends message it puts there locally unique sequence 
> >  * and random acknowledge numbers.
> >  * Sequence number may be copied into nlmsghdr->nlmsg_seq too.
> >  *
> >  * Sequence number is incremented with each message to be sent.
> >  *
> >  * If we expect reply to our message, 
> >  * then sequence number in received message MUST be the same as in original message,
> >  * and acknowledge number MUST be the same + 1.
> >  *
> >  * If we receive message and it's sequence number is not equal to one we are expecting, 
> >  * then it is new message.
> >  * If we receive message and it's sequence number is the same as one we are expecting,
> >  * but it's acknowledge is not equal acknowledge number in original message + 1,
> >  * then it is new message.
> >  *
> >  */
> 
> This comment looks crappy in an 80-col xterm.

Does anyone still use it? :)

> What happens if we expect a reply to our message but userspace never sends
> one?  Does the kernel leak memory?  Do other processes hang?

It is only advice, one may easily skip seq/ack initialization.
I could remove it totally from the header, but decided to 
place it to force people to use more reliable protocols over netlink
by introducing such overhead.

> > void cn_netlink_send(struct cn_msg *msg, u32 __groups)
> > {
> > 	struct cn_callback_entry *n, *__cbq;
> > 	unsigned int size;
> > 	struct sk_buff *skb, *uskb;
> > 	struct nlmsghdr *nlh;
> > 	struct cn_msg *data;
> > 	struct cn_dev *dev = &cdev;
> > 	u32 groups = 0;
> > 	int found = 0;
> > 
> > 	if (!__groups)
> > 	{
> 
> Wrong indenting.

Yep, my fault...

> > 		spin_lock_bh(&dev->cbdev->queue_lock);
> > 		list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> > 			if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> > 				found = 1;
> > 				groups = __cbq->group;
> > 			}
> > 		}
> > 		spin_unlock_bh(&dev->cbdev->queue_lock);
> > 
> > 		if (!found) {
> > 			printk(KERN_ERR "Failed to find multicast netlink group for callback[0x%x.0x%x]. seq=%u\n",
> > 			       msg->id.idx, msg->id.val, msg->seq);
> 
> Needs wrapping.
> 
> > 			return;
> > 		}
> > 	}
> > 	else
> > 		groups = __groups;
> > 
> > 	size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> > 
> > 	skb = alloc_skb(size, GFP_ATOMIC);
> 
> GFP_ATOMIC is quite unreliable.  Better to find a way to use GFP_KERNEL here.

It can be used in bh context, my first TODO entryf for connector is
provideing
extended API with GFP flags provided by caller.

> > 	if (!skb) {
> > 		printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> > 		return;
> > 	}
> 
> Surely we should return -ENOMEM here?  How is the caller to know that the
> send attempt worked?

It is notification area, notification may fail and original applicant
can not 
and should not know about it.
If it was direct call, then error must be propagated to the caller, 
but indirect notification is not.

> > 	nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> > 
> > 	data = (struct cn_msg *)NLMSG_DATA(nlh);
> 
> Unneeded typecast.

Is it really an issue?
I try to always dereference void pointer to the given type...

> > 	memcpy(data, msg, sizeof(*data) + msg->len);
> > #if 0
> > 	printk("%s: len=%u, seq=%u, ack=%u, group=%u.\n",
> > 	       __func__, msg->len, msg->seq, msg->ack, groups);
> > #endif
> > 	
> > 	NETLINK_CB(skb).dst_groups = groups;
> > 
> > 	uskb = skb_clone(skb, GFP_ATOMIC);
> > 	if (uskb) {
> > 		netlink_unicast(dev->nls, uskb, 0, 0);
> > 	}
> 
> Unneeded {}

Ok.

> > 	netlink_broadcast(dev->nls, skb, 0, groups, GFP_ATOMIC);
> 
> GFP_ATOMIC is quite undesirable.

It will be changed to provided GFP flags by caller soon.

> > 
> > static int cn_call_callback(struct cn_msg *msg, void (*destruct_data) (void *), void *data)
> 
> 80 cols
> 
> > {
> > 	struct cn_callback_entry *n, *__cbq;
> > 	struct cn_dev *dev = &cdev;
> > 	int found = 0;
> > 
> > 	spin_lock_bh(&dev->cbdev->queue_lock);
> > 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 
> 80 cols

Grrr....

> > 		if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> > 			__cbq->cb->priv = msg;
> > 
> > 			__cbq->ddata = data;
> > 			__cbq->destruct_data = destruct_data;
> > 
> > 			queue_work(dev->cbdev->cn_queue, &__cbq->work);
> > 			found = 1;
> > 			break;
> > 		}
> > 	}
> > 	spin_unlock_bh(&dev->cbdev->queue_lock);
> > 
> > 	return found;
> > }
> 
> Why is spin_lock_bh() being used here?

skb may be delivered in soft irq context, and may race with sending.
And actually it can be sent from irq context, like it is done in test
module.

> > static int __cn_rx_skb(struct sk_buff *skb, struct nlmsghdr *nlh)
> > {
> > 	u32 pid, uid, seq, group;
> > 	struct cn_msg *msg;
> > 
> > 	pid = NETLINK_CREDS(skb)->pid;
> > 	uid = NETLINK_CREDS(skb)->uid;
> > 	seq = nlh->nlmsg_seq;
> > 	group = NETLINK_CB((skb)).groups;
> > 	msg = (struct cn_msg *)NLMSG_DATA(nlh);
> > 
> > 	if (NLMSG_SPACE(msg->len + sizeof(*msg)) != nlh->nlmsg_len) {
> > 		printk(KERN_ERR "skb does not have enough length: "
> > 				"requested msg->len=%u[%u], nlh->nlmsg_len=%u, skb->len=%u.\n",
> 
> 80 cols (all over the place)

Ok...

> > static void cn_notify(struct cb_id *id, u32 notify_event)
> > {
> > 	struct cn_ctl_entry *ent;
> > 
> > 	spin_lock_bh(&notify_lock);
> > 	list_for_each_entry(ent, &notify_list, notify_entry) {
> > 		int i;
> > 		struct cn_notify_req *req;
> > 		struct cn_ctl_msg *ctl = ent->msg;
> > 		int a, b;
> > 
> > 		a = b = 0;
> > 		
> > 		req = (struct cn_notify_req *)ctl->data;
> > 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {
> 
> 		for (i = 0; i < ctl->idx_notify_num; i++, req++) {
> 
> > 			if (id->idx >= req->first && id->idx < req->first + req->range) {
> > 				a = 1;
> > 				break;
> > 			}
> > 		}
> > 		
> > 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> > 			if (id->val >= req->first && id->val < req->first + req->range) {
> > 				b = 1;
> > 				break;
> > 			}
> > 		}
> > 
> > 		if (a && b) {
> > 			struct cn_msg m;
> > 			
> > 			printk(KERN_INFO "Notifying group %x with event %u about %x.%x.\n", 
> > 					ctl->group, notify_event, 
> > 					id->idx, id->val);
> > 
> > 			memset(&m, 0, sizeof(m));
> > 			m.ack = notify_event;
> > 
> > 			memcpy(&m.id, id, sizeof(m.id));
> > 			cn_netlink_send(&m, ctl->group);
> > 		}
> 
> What's all the above code doing?  What do `a' and `b' mean?  Needs
> commentary and better-chosen identifiers.

It searches for idx and val to match requested notification, 
if "a" is true - idx is found, if b - val is found.

> > 	}
> > 	spin_unlock_bh(&notify_lock);
> > }
> > 
> > int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *))
> > {
> > 	int err;
> > 	struct cn_dev *dev = &cdev;
> > 	struct cn_callback *cb;
> > 
> > 	cb = kmalloc(sizeof(*cb), GFP_KERNEL);
> > 	if (!cb) {
> > 		printk(KERN_INFO "%s: Failed to allocate new struct cn_callback.\n",
> > 		       dev->cbdev->name);
> > 		return -ENOMEM;
> > 	}
> > 
> > 	memset(cb, 0, sizeof(*cb));
> > 
> > 	snprintf(cb->name, sizeof(cb->name), "%s", name);
> 
> scnprintf?

Returned value is ignored here, so they do not differ.
Will change it though.

> > 	memcpy(&cb->id, id, sizeof(cb->id));
> > 	cb->callback = callback;
> > 
> > 	atomic_set(&cb->refcnt, 0);
> > 
> > 	err = cn_queue_add_callback(dev->cbdev, cb);
> > 	if (err) {
> > 		kfree(cb);
> > 		return err;
> > 	}
> > 			
> > 	cn_notify(id, 0);
> > 
> > 	return 0;
> > }
> > 
> > void cn_del_callback(struct cb_id *id)
> > {
> > 	struct cn_dev *dev = &cdev;
> > 	struct cn_callback_entry *n, *__cbq;
> > 
> > 	list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> > 		if (cn_cb_equal(&__cbq->cb->id, id)) {
> > 			cn_queue_del_callback(dev->cbdev, __cbq->cb);
> > 			cn_notify(id, 1);
> > 			break;
> > 		}
> > 	}
> > }
> 
> Doesn't this list walk need locking?

Ugh, you are tight, I totally wrong here.
It requires dev->queue_lock to be hold.

> Please document all functions with comments.  Functions which constitute
> part of the external API should be commented using the kernel-doc format.

There is Documentation/connector/connector.txt which describes all
exported functions and structures.
Should it be ported to docbook?

> > static void cn_callback(void * data)
> > {
> > 	struct cn_msg *msg = (struct cn_msg *)data;
> > 	struct cn_ctl_msg *ctl;
> > 	struct cn_ctl_entry *ent;
> > 	u32 size;
> >  
> > 	if (msg->len < sizeof(*ctl)) {
> > 		printk(KERN_ERR "Wrong connector request size %u, must be >= %u.\n", 
> > 				msg->len, sizeof(*ctl));
> > 		return;
> > 	}
> > 	
> > 	ctl = (struct cn_ctl_msg *)msg->data;
> > 
> > 	size = sizeof(*ctl) + (ctl->idx_notify_num + ctl->val_notify_num)*sizeof(struct cn_notify_req);
> > 
> > 	if (msg->len != size) {
> > 		printk(KERN_ERR "Wrong connector request size %u, must be == %u.\n", 
> > 				msg->len, size);
> > 		return;
> > 	}
> > 
> > 	if (ctl->len + sizeof(*ctl) != msg->len) {
> > 		printk(KERN_ERR "Wrong message: msg->len=%u must be equal to inner_len=%u [+%u].\n", 
> > 				msg->len, ctl->len, sizeof(*ctl));
> > 		return;
> > 	}
> > 
> > 	/*
> > 	 * Remove notification.
> > 	 */
> > 	if (ctl->group == 0) {
> > 		struct cn_ctl_entry *n;
> > 		
> > 		spin_lock_bh(&notify_lock);
> > 		list_for_each_entry_safe(ent, n, &notify_list, notify_entry) {
> > 			if (cn_ctl_msg_equals(ent->msg, ctl)) {
> > 				list_del(&ent->notify_entry);
> > 				kfree(ent);
> > 			}
> > 		}
> > 		spin_unlock_bh(&notify_lock);
> > 
> > 		return;
> > 	}
> > 
> > 	size += sizeof(*ent);
> > 
> > 	ent = kmalloc(size, GFP_ATOMIC);
> 
> Another GFP_ATOMIC :(  In what context can this function be called?

It is called from BH context.
It is only one place that can not be moved to the different GFP
allocation,
but it is notification area, that is quite rarely used as fas as I can
see...

> > 	{
> > 		int i;
> > 		struct cn_notify_req *req;
> 
> May as well move these up to the top of the function, save the ugly indent.
> 
> > 		printk("Notify group %x for idx: ", ctl->group);
> > 
> > 		req = (struct cn_notify_req *)ctl->data;
> > 		for (i=0; i<ctl->idx_notify_num; ++i, ++req) {
> > 			printk("%u-%u ", req->first, req->first+req->range-1);
> > 		}
> 
> Unneeded braces.
> 
> This file is full of trailing whitespace, btw.  Please fix that up, then
> find a new editor.
> 
> > 		printk("\nNotify group %x for val: ", ctl->group);
> > 
> > 		for (i=0; i<ctl->val_notify_num; ++i, ++req) {
> > 			printk("%u-%u ", req->first, req->first+req->range-1);
> > 		}
> 
> Braces.

It is debug code and I will remove it.

-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

Attachment: signature.asc
Description: This is a digitally signed message part