Re: [Fwd: Re: connector is missing in 2.6.12-rc2-mm1]

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

On Sun, Apr 10, 2005 at 09:27:27PM +0200, Thomas Graf (tgraf@suug.ch) wrote:
> * jamal <1113143959.1089.316.camel@jzny.localdomain> 2005-04-10 10:39
> > Please crosspost on netdev - you should know that by now;->
> > 
> > I actually disagreee with Herbert on this. Theres definetely good
> > need to have a more usable messaging system that rides on top of
> > netlink. It is not that netlink cant be extended (I actually think thats
> > a separate topic)
> 
> I find it quite easy already but I guess a few macros would improve
> it even more. The routing attribute macros could be made generic to
> so can benefit from the advanages of TLVs.
> 
> Evgeniy, Sorry for not having time earlier to give your patch a
> review. I'm not yet through completely and won't comment on the
> overall architecture until I have understood it all.
> 
> diff -Nru /tmp/empty/cn_queue.c linux-2.6/drivers/connector/cn_queue.c
> --- /tmp/empty/cn_queue.c       1970-01-01 03:00:00.000000000 +0300
> +++ linux-2.6/drivers/connector/cn_queue.c      2004-09-24 00:01:00.000000000 
> +int cn_queue_add_callback(struct cn_queue_dev *dev, struct cn_callback *cb)
> +{
> +       struct cn_callback_entry *cbq, *n, *__cbq;
> +       int found = 0;
> +
> +       cbq = cn_queue_alloc_callback_entry(cb);
> +       if (!cbq)
> +               return -ENOMEM;
> +
> +       atomic_inc(&dev->refcnt);
> +       cbq->pdev = dev;
> +
> +       spin_lock(&dev->queue_lock);
> +       list_for_each_entry_safe(__cbq, n, &dev->queue_list, callback_entry) {
> 
> Why _safe? There is no way a entry can be removed here.

No particular reason - it easier to copy-paste it from other line :)

> +               if (cn_cb_equal(&__cbq->cb->id, &cb->id)) {
> +                       found = 1;
> +                       break;
> +               }
> +       }
> diff -Nru /tmp/empty/connector.c linux-2.6/drivers/connector/connector.c
> --- /tmp/empty/connector.c      1970-01-01 03:00:00.000000000 +0300
> +++ linux-2.6/drivers/connector/connector.c     2004-09-24 00:01:00.000000000 
> +void cn_netlink_send(struct cn_msg *msg, u32 __groups)
> +{
> +       struct cn_callback_entry *n, *__cbq;
> +       unsigned int size;
> +       struct sk_buff *skb;
> +       struct nlmsghdr *nlh;
> +       struct cn_msg *data;
> +       struct cn_dev *dev = &cdev;
> +       u32 groups = 0;
> +       int found = 0;
> +
> +       if (!__groups)
> +       {
> +               spin_lock(&dev->cbdev->queue_lock);
> +               list_for_each_entry_safe(__cbq, n, &dev->cbdev->queue_list, callback_entry) {
> 
> Same here
> 
> +                       if (cn_cb_equal(&__cbq->cb->id, &msg->id)) {
> +                               found = 1;
> +                               groups = __cbq->group;
> +                       }
> +               }
> +               spin_unlock(&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);
> +                       return;
> +               }
> +       }
> +       else
> +               groups = __groups;
> +
> +       size = NLMSG_SPACE(sizeof(*msg) + msg->len);
> +
> +       skb = alloc_skb(size, GFP_ATOMIC);
> +       if (!skb) {
> +               printk(KERN_ERR "Failed to allocate new skb with size=%u.\n", size);
> +               return;
> +       }
> +
> +       nlh = NLMSG_PUT(skb, 0, msg->seq, NLMSG_DONE, size - sizeof(*nlh));
> 
> This is not correct, what happens is:
> size = NLMSG_SPACE(sizeof(*msg) + msg->len);
>  --> align(hdr)+align(data)
> size - sizeof(*nlh)
>  --> (align(hdr)-hdr)+align(data)
> NLMSG_PUT pads again to get to the end of the data block (NLMSG_LENGTH)
>  --> align(hdr)+(align(hdr)-hdr)+align(data)
> 
> At the moment align(hdr) == hdr since nlmsghdr is already aligned
> but this might change and your code will break.

As far as I remember, header is always supposed to be aligned properly
"by design", so it even could be nonaligned here.

-- 
	Evgeniy Polyakov ( s0mbre )