Subject: Re: [RFCv3 bluetooth-next 4/4] 6lowpan: iphc: add support for stateful compression

From: Stefan Schmidt <[email-masked]>
Date: Fri, 4 Dec 2015 13:13:02 +0100

Hello.

On 03/12/15 15:22, Alexander Aring wrote:
> Hi,
>
> On Wed, Dec 02, 2015 at 03:18:06PM +0100, Stefan Schmidt wrote:
>> Hello.
>>
>> A bit more review here. Still haven't tested the code.
>>
>> On 29/11/15 12:34, Alexander Aring wrote:
>>> This patch introduce support for IPHC stateful address compression. It
>>> will offer three debugfs per interface entries, which are:
>>>
>>>   - dci_table: destination context indentifier table
>>>   - sci_table: source context indentifier table
>>>   - mcast_sci_table: multicast context identifier table
>>>
>>> Example to setup a context id:
>>>
>>> A "cat /sys/kernel/debug/6lowpan/lowpan0/dci_table" will display all
>>> contexts which are available. Example:
>>>
>>> ID ipv6-address/prefix-length                  enabled
>>> 0  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 1  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 2  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 3  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 4  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 5  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 6  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 7  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 8  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 9  0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 10 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 11 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 12 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 13 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 14 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>> 15 0000:0000:0000:0000:0000:0000:0000:0000/0   0
>>>
>>> For setting a context e.g. context id 0, context 2001::, prefix-length
>>> 64.
>>>
>>> Hint: Simple copy one line and then maniuplate it.
>>>
>>> echo "0 2001:0000:0000:0000:0000:0000:0000:0000/64 1" >
>>> /sys/kernel/debug/6lowpan/lowpan0/dci_table
>>>
>>> The enabled column will display currently if the context is disabled(0) or
>>> enabled(1).
>>>
>>> On transmit side:
>>>
>>> The IPHC code will automatically search for a context which would be
>>> match for the address. Then it will be use the context with the
>>> best compression method. Means the longest prefix which match will be
>>> used.
>>>
>>> Example:
>>>
>>> 2001::/126 vs 2001::/127 - the 2001::/127 can be full compressed if the
>>> last bit of the address which has the prefix 2001::/127 is the same like
>>> the IID from the Encapsulating Header. A context ID can also be a
>>> 2001::1/128, which is then a full ipv6 address.
>>>
>>> On Receive side:
>>>
>>> If there is a context defined (when CID not available then it's the
>>> default context 0) then it will be used, if the header doesn't set
>>> SAC or DAC bit thens, it will be dropped.
>>>
>>> Signed-off-by: Alexander Aring <[email-masked]>
>>> ---
>>>   include/net/6lowpan.h   |  54 ++++++
>>>   net/6lowpan/6lowpan_i.h |   3 +
>>>   net/6lowpan/core.c      |  17 +-
>>>   net/6lowpan/debugfs.c   | 113 ++++++++++++
>>>   net/6lowpan/iphc.c      | 479 ++++++++++++++++++++++++++++++++++++++++++------
>>>   5 files changed, 608 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>>> index 2f6a3f2..dec5427 100644
>>> --- a/include/net/6lowpan.h
>>> +++ b/include/net/6lowpan.h
>>> @@ -75,6 +75,8 @@
>>>   #define LOWPAN_IPHC_MAX_HC_BUF_LEN	(sizeof(struct ipv6hdr) +	\
>>>   					 LOWPAN_IPHC_MAX_HEADER_LEN +	\
>>>   					 LOWPAN_NHC_MAX_HDR_LEN)
>>> +/* SCI/DCI is 4 bit width, so we have maximum 16 entries */
>>> +#define LOWPAN_IPHC_CI_TABLE_SIZE	(1 << 4)
>>>   #define LOWPAN_DISPATCH_IPV6		0x41 /* 01000001 = 65 */
>>>   #define LOWPAN_DISPATCH_IPHC		0x60 /* 011xxxxx = ... */
>>> @@ -98,9 +100,37 @@ enum lowpan_lltypes {
>>>   	LOWPAN_LLTYPE_IEEE802154,
>>>   };
>>> +struct lowpan_iphc_ctx {
>>> +	u8 id;
>>> +	struct in6_addr addr;
>>> +	u8 prefix_len;
>>> +	bool enabled;
>>> +};
>>> +
>>> +struct lowpan_iphc_ctx_ops {
>>> +	bool (*valid_prefix)(const struct lowpan_iphc_ctx *ctx);
>>> +	int (*update)(struct lowpan_iphc_ctx *table,
>>> +		      const struct lowpan_iphc_ctx *ctx);
>>> +	struct lowpan_iphc_ctx *(*get_by_id)(const struct net_device *dev,
>>> +					     struct lowpan_iphc_ctx *table,
>>> +					     u8 id);
>>> +	struct lowpan_iphc_ctx *(*get_by_addr)(const struct net_device *dev,
>>> +					       struct lowpan_iphc_ctx *table,
>>> +					       const struct in6_addr *addr);
>>> +};
>>> +
>>> +struct lowpan_iphc_ctx_table {
>>> +	spinlock_t lock;
>>> +	const struct lowpan_iphc_ctx_ops *ops;
>>> +	struct lowpan_iphc_ctx table[LOWPAN_IPHC_CI_TABLE_SIZE];
>>> +};
>>> +
>>>   struct lowpan_priv {
>>>   	enum lowpan_lltypes lltype;
>>>   	struct dentry *iface_debugfs;
>>> +	struct lowpan_iphc_ctx_table iphc_dci;
>>> +	struct lowpan_iphc_ctx_table iphc_sci;
>>> +	struct lowpan_iphc_ctx_table iphc_mcast_dci;
>>>   	/* must be last */
>>>   	u8 priv[0] __aligned(sizeof(void *));
>>> @@ -125,6 +155,30 @@ struct lowpan_802154_cb *lowpan_802154_cb(const struct sk_buff *skb)
>>>   	return (struct lowpan_802154_cb *)skb->cb;
>>>   }
>>> +static inline int lowpan_ctx_update(struct lowpan_iphc_ctx_table *t,
>>> +				    const struct lowpan_iphc_ctx *ctx)
>>> +{
>>> +	if (!t->ops->valid_prefix(ctx))
>>> +		return -EINVAL;
>>> +
>>> +	return t->ops->update(t->table, ctx);
>>> +}
>>> +
>>> +static inline struct lowpan_iphc_ctx *
>>> +lowpan_ctx_by_id(const struct net_device *dev, struct lowpan_iphc_ctx_table *t,
>>> +		 u8 id)
>>> +{
>>> +	return t->ops->get_by_id(dev, t->table, id);
>>> +}
>>> +
>>> +static inline struct lowpan_iphc_ctx *
>>> +lowpan_ctx_by_addr(const struct net_device *dev,
>>> +		   struct lowpan_iphc_ctx_table *t,
>>> +		   const struct in6_addr *addr)
>>> +{
>>> +	return t->ops->get_by_addr(dev, t->table, addr);
>>> +}
>>> +
>>>   #ifdef DEBUG
>>>   /* print data in line */
>>>   static inline void raw_dump_inline(const char *caller, char *msg,
>>> diff --git a/net/6lowpan/6lowpan_i.h b/net/6lowpan/6lowpan_i.h
>>> index d16bb4b..2c275be 100644
>>> --- a/net/6lowpan/6lowpan_i.h
>>> +++ b/net/6lowpan/6lowpan_i.h
>>> @@ -3,6 +3,9 @@
>>>   #include <linux/netdevice.h>
>>> +extern const struct lowpan_iphc_ctx_ops iphc_ctx_unicast_ops;
>>> +extern const struct lowpan_iphc_ctx_ops iphc_ctx_mcast_ops;
>>> +
>>>   #ifdef CONFIG_6LOWPAN_DEBUGFS
>>>   int lowpan_dev_debugfs_init(struct net_device *dev);
>>>   void lowpan_dev_debugfs_exit(struct net_device *dev);
>>> diff --git a/net/6lowpan/core.c b/net/6lowpan/core.c
>>> index c7f06f5..f3dbd18 100644
>>> --- a/net/6lowpan/core.c
>>> +++ b/net/6lowpan/core.c
>>> @@ -20,7 +20,7 @@
>>>   int lowpan_register_netdevice(struct net_device *dev,
>>>   			      enum lowpan_lltypes lltype)
>>>   {
>>> -	int ret;
>>> +	int i, ret;
>>>   	dev->addr_len = EUI64_ADDR_LEN;
>>>   	dev->type = ARPHRD_6LOWPAN;
>>> @@ -29,6 +29,21 @@ int lowpan_register_netdevice(struct net_device *dev,
>>>   	lowpan_priv(dev)->lltype = lltype;
>>> +	spin_lock_init(&lowpan_priv(dev)->iphc_dci.lock);
>>> +	lowpan_priv(dev)->iphc_dci.ops = &iphc_ctx_unicast_ops;
>>> +
>>> +	spin_lock_init(&lowpan_priv(dev)->iphc_sci.lock);
>>> +	lowpan_priv(dev)->iphc_sci.ops = &iphc_ctx_unicast_ops;
>>> +
>>> +	spin_lock_init(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +	lowpan_priv(dev)->iphc_mcast_dci.ops = &iphc_ctx_mcast_ops;
>>> +
>>> +	for (i = 0; i < LOWPAN_IPHC_CI_TABLE_SIZE; i++) {
>>> +		lowpan_priv(dev)->iphc_dci.table[i].id = i;
>>> +		lowpan_priv(dev)->iphc_sci.table[i].id = i;
>>> +		lowpan_priv(dev)->iphc_mcast_dci.table[i].id = i;
>>> +	}
>>> +
>>>   	ret = lowpan_dev_debugfs_init(dev);
>>>   	if (ret < 0)
>>>   		return ret;
>>> diff --git a/net/6lowpan/debugfs.c b/net/6lowpan/debugfs.c
>>> index 88eef84..24f94ac 100644
>>> --- a/net/6lowpan/debugfs.c
>>> +++ b/net/6lowpan/debugfs.c
>>> @@ -16,19 +16,132 @@
>>>   #include "6lowpan_i.h"
>>> +#define LOWPAN_DEBUGFS_CTX_NUM_ARGS	11
>>> +
>>>   static struct dentry *lowpan_debugfs;
>>> +static int lowpan_context_show(struct seq_file *file, void *offset)
>>> +{
>>> +	struct lowpan_iphc_ctx_table *t = file->private;
>>> +	int i;
>>> +
>>> +	seq_printf(file, "%-2s %-43s %s\n", "ID", "ipv6-address/prefix-length",
>>> +		   "enabled");
>>> +
>>> +	spin_lock_bh(&t->lock);
>>> +	for (i = 0; i < LOWPAN_IPHC_CI_TABLE_SIZE; i++) {
>>> +		seq_printf(file,
>>> +			   "%-2d %04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x/%-3d %d\n",
>>> +			   t->table[i].id,
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[0]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[1]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[2]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[3]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[4]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[5]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[6]),
>>> +			   be16_to_cpu(t->table[i].addr.s6_addr16[7]),
>>> +			   t->table[i].prefix_len, t->table[i].enabled);
>>> +	}
>>> +	spin_unlock_bh(&t->lock);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int lowpan_context_dbgfs_open(struct inode *inode, struct file *file)
>>> +{
>>> +	return single_open(file, lowpan_context_show, inode->i_private);
>>> +}
>>> +
>>> +static ssize_t lowpan_context_dbgfs_write(struct file *fp,
>>> +					  const char __user *user_buf,
>>> +					  size_t count, loff_t *ppos)
>>> +{
>>> +	char buf[128] = { };
>> Do we enforce the 128 char limit somewhere?
>>
>>> +	struct seq_file *file = fp->private_data;
>>> +	struct lowpan_iphc_ctx_table *t = file->private;
>>> +	struct lowpan_iphc_ctx ctx;
>>> +	int status = count, n, id, enabled, i, prefix_len, ret;
>>> +	unsigned int addr[8];
>>> +
>>> +	if (copy_from_user(&buf, user_buf, min_t(size_t, sizeof(buf) - 1,
>>> +						 count))) {
>> I see we do here, ok.
>>
>>> +		status = -EFAULT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	n = sscanf(buf, "%d %04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x/%d %d",
>>> +		   &id, &addr[0], &addr[1], &addr[2], &addr[3], &addr[4],
>>> +		   &addr[5], &addr[6], &addr[7], &prefix_len, &enabled);
>>> +	if (n != LOWPAN_DEBUGFS_CTX_NUM_ARGS) {
>>> +		status = -EIO;
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (id > LOWPAN_IPHC_CI_TABLE_SIZE - 1 ||
>>> +	    (enabled != 0 && enabled != 1) || prefix_len > 128) {
>> Hmm, if you would use a bool for enabled here instead of an int the check on
>> enabled would not be needed.
>>
> This is because sscanf format string excepts an int here to parsing the
> "%d" argument.
>
> I adapt the same mechanism here what we have for "bools" for nl802154.
>
> When I change it to bool, I will get:
>
> net/6lowpan/debugfs.c: In function 'lowpan_context_dbgfs_write':
> net/6lowpan/debugfs.c:76:6: warning: format '%d' expects argument of
> type 'int *', but argument 13 has type 'bool *' [-Wformat=]
>        &addr[5], &addr[6], &addr[7], &prefix_len, &enabled);

Ah, right, scanf has no support for bool. :( Guess we keep it as int.

>
>>> +		status = -EINVAL;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ctx.id = id;
>>> +	ctx.enabled = enabled;
> conversion to bool is here, I could do "!!enabled" and remove the check
> above, but then is 1 <-> MAX_INT and -1 <-> MIN_INT also true.

Nah, leave it as is. I was under the impüression we could use scanf for 
bool directly. That not being the case we can keep it as is.

>
>>> +	ctx.prefix_len = prefix_len;
>>> +
>>> +	for (i = 0; i < 8; i++)
>>> +		ctx.addr.s6_addr16[i] = cpu_to_be16(addr[i] & 0xffff);
>>> +
>>> +	spin_lock_bh(&t->lock);
>>> +	ret = lowpan_ctx_update(t, &ctx);
>>> +	if (ret < 0)
>>> +		status = ret;
>>> +	spin_unlock_bh(&t->lock);
>>> +
>>> +out:
>>> +	return status;
>>> +}
>>> +
>>> +const struct file_operations lowpan_context_fops = {
>>> +	.open		= lowpan_context_dbgfs_open,
>>> +	.read		= seq_read,
>>> +	.write		= lowpan_context_dbgfs_write,
>>> +	.llseek		= seq_lseek,
>>> +	.release	= single_release,
>>> +};
>>> +
>>>   int lowpan_dev_debugfs_init(struct net_device *dev)
>>>   {
>>>   	struct lowpan_priv *lpriv = lowpan_priv(dev);
>>> +	static struct dentry *dentry;
>>>   	/* creating the root */
>>>   	lpriv->iface_debugfs = debugfs_create_dir(dev->name, lowpan_debugfs);
>>>   	if (!lpriv->iface_debugfs)
>>>   		goto fail;
>>> +	dentry = debugfs_create_file("dci_table", 0664, lpriv->iface_debugfs,
>> Any special reason you want the group have write permissions? I would expect
>> a 644 here. Same for the other two.
>>
> No, I will change it to 0644.

Great, thanks.

>
>>> +				     &lowpan_priv(dev)->iphc_dci,
>>> +				     &lowpan_context_fops);
>>> +	if (!dentry)
>>> +		goto remove_root;
>>> +
>>> +	dentry = debugfs_create_file("sci_table", 0664, lpriv->iface_debugfs,
>>> +				     &lowpan_priv(dev)->iphc_sci,
>>> +				     &lowpan_context_fops);
>>> +	if (!dentry)
>>> +		goto remove_root;
>>> +
>>> +	dentry = debugfs_create_file("mcast_dci_table", 0664,
>>> +				     lpriv->iface_debugfs,
>>> +				     &lowpan_priv(dev)->iphc_mcast_dci,
>>> +				     &lowpan_context_fops);
>>> +	if (!dentry)
>>> +		goto remove_root;
>>> +
>>>   	return 0;
>>> +remove_root:
>>> +	lowpan_dev_debugfs_exit(dev);
>> Just to check here. This one is calling debugfs_remove_recursive(). This
>> will cleanup states where we might have been able to create dci_table and
>> sci_table but not mcast_dci_table, right?
> Yes, should. I will remove the whole interface related debugfs entry
> inclusive all sub-entries which was created under the interface related
> directory entry which is in this case sci/dci/mcast_dci tables.

OK

>>>   fail:
>>>   	return -EINVAL;
>>>   }
>>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>>> index 346b5c1..2c5004b 100644
>>> --- a/net/6lowpan/iphc.c
>>> +++ b/net/6lowpan/iphc.c
>>> @@ -56,6 +56,7 @@
>>>   /* special link-layer handling */
>>>   #include <net/mac802154.h>
>>> +#include "6lowpan_i.h"
>>>   #include "nhc.h"
>>>   /* Values of fields within the IPHC encoding first byte */
>>> @@ -147,6 +148,9 @@
>>>   	 (((a)->s6_addr16[6]) == 0) &&		\
>>>   	 (((a)->s6_addr[14]) == 0))
>>> +#define LOWPAN_IPHC_CID_DCI(cid)	(cid & 0x0f)
>>> +#define LOWPAN_IPHC_CID_SCI(cid)	((cid & 0xf0) >> 4)
>>> +
>>>   static inline void iphc_uncompress_eui64_lladdr(struct in6_addr *ipaddr,
>>>   						const void *lladdr)
>>>   {
>>> @@ -259,30 +263,59 @@ static int uncompress_addr(struct sk_buff *skb, const struct net_device *dev,
>>>   /* Uncompress address function for source context
>>>    * based address(non-multicast).
>>>    */
>>> -static int uncompress_context_based_src_addr(struct sk_buff *skb,
>>> -					     struct in6_addr *ipaddr,
>>> -					     u8 address_mode)
>>> +static int uncompress_ctx_addr(struct sk_buff *skb,
>>> +			       const struct net_device *dev,
>>> +			       const struct lowpan_iphc_ctx *ctx,
>>> +			       struct in6_addr *ipaddr, u8 address_mode,
>>> +			       const void *lladdr)
>>>   {
>>> +	bool fail;
>>> +
>>>   	switch (address_mode) {
>>> -	case LOWPAN_IPHC_SAM_00:
>>> -		/* unspec address ::
>>> +	/* SAM and DAM are the same here */
>>> +	case LOWPAN_IPHC_DAM_00:
>>> +		fail = false;
>>> +		/* SAM_00 -> unspec address ::
>>>   		 * Do nothing, address is already ::
>>> +		 *
>>> +		 * DAM 00 -> reserved should never occur.
>>>   		 */
>>>   		break;
>>>   	case LOWPAN_IPHC_SAM_01:
>>> -		/* TODO */
>>> +	case LOWPAN_IPHC_DAM_01:
>>> +		fail = lowpan_fetch_skb(skb, &ipaddr->s6_addr[8], 8);
>>> +		ipv6_addr_prefix_copy(ipaddr, &ctx->addr, ctx->prefix_len);
>>> +		break;
>>>   	case LOWPAN_IPHC_SAM_10:
>>> -		/* TODO */
>>> +	case LOWPAN_IPHC_DAM_10:
>>> +		ipaddr->s6_addr[11] = 0xFF;
>>> +		ipaddr->s6_addr[12] = 0xFE;
>>> +		fail = lowpan_fetch_skb(skb, &ipaddr->s6_addr[14], 2);
>>> +		ipv6_addr_prefix_copy(ipaddr, &ctx->addr, ctx->prefix_len);
>>> +		break;
>>>   	case LOWPAN_IPHC_SAM_11:
>>> -		/* TODO */
>>> -		netdev_warn(skb->dev, "SAM value 0x%x not supported\n",
>>> -			    address_mode);
>>> -		return -EINVAL;
>>> +	case LOWPAN_IPHC_DAM_11:
>>> +		fail = false;
>>> +		switch (lowpan_priv(dev)->lltype) {
>>> +		case LOWPAN_LLTYPE_IEEE802154:
>>> +			iphc_uncompress_802154_lladdr(ipaddr, lladdr);
>>> +			break;
>>> +		default:
>>> +			iphc_uncompress_eui64_lladdr(ipaddr, lladdr);
>>> +			break;
>>> +		}
>>> +		ipv6_addr_prefix_copy(ipaddr, &ctx->addr, ctx->prefix_len);
>>> +		break;
>>>   	default:
>>>   		pr_debug("Invalid sam value: 0x%x\n", address_mode);
>>>   		return -EINVAL;
>>>   	}
>>> +	if (fail) {
>>> +		pr_debug("Failed to fetch skb data\n");
>>> +		return -EIO;
>>> +	}
>>> +
>>>   	raw_dump_inline(NULL,
>>>   			"Reconstructed context based ipv6 src addr is",
>>>   			ipaddr->s6_addr, 16);
>>> @@ -346,6 +379,33 @@ static int lowpan_uncompress_multicast_daddr(struct sk_buff *skb,
>>>   	return 0;
>>>   }
>>> +static int lowpan_uncompress_multicast_ctx_daddr(struct sk_buff *skb,
>>> +						 struct lowpan_iphc_ctx *ctx,
>>> +						 struct in6_addr *ipaddr,
>>> +						 u8 address_mode)
>>> +{
>>> +	struct in6_addr network_pfx = {};
>>> +	bool fail;
>>> +
>>> +	if (address_mode != LOWPAN_IPHC_DAM_00)
>>> +		return -EINVAL;
>>> +
>>> +	ipaddr->s6_addr[0] = 0xFF;
>>> +	fail = lowpan_fetch_skb(skb, &ipaddr->s6_addr[1], 2);
>>> +	fail |= lowpan_fetch_skb(skb, &ipaddr->s6_addr[12], 4);
>>> +	/* take prefix_len and network prefix from the context */
>>> +	ipaddr->s6_addr[3] = ctx->prefix_len;
>>> +	/* get network prefix to copy into multicast address */
>>> +	ipv6_addr_prefix(&network_pfx, &ctx->addr, ctx->prefix_len);
>>> +	/* setting network prefix */
>>> +	memcpy(&ipaddr->s6_addr[4], &network_pfx, 8);
>>> +
>>> +	if (fail < 0)
>>> +		return -EIO;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   /* get the ecn values from iphc tf format and set it to ipv6hdr */
>>>   static inline void lowpan_iphc_tf_set_ecn(struct ipv6hdr *hdr, const u8 *tf)
>>>   {
>>> @@ -459,7 +519,8 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>>>   			     const void *daddr, const void *saddr)
>>>   {
>>>   	struct ipv6hdr hdr = {};
>>> -	u8 iphc0, iphc1;
>>> +	struct lowpan_iphc_ctx *ci;
>>> +	u8 iphc0, iphc1, cid = 0;
>>>   	int err;
>>>   	raw_dump_table(__func__, "raw skb data dump uncompressed",
>>> @@ -469,12 +530,14 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>>>   	    lowpan_fetch_skb(skb, &iphc1, sizeof(iphc1)))
>>>   		return -EINVAL;
>>> -	/* another if the CID flag is set */
>>> -	if (iphc1 & LOWPAN_IPHC_CID)
>>> -		return -ENOTSUPP;
>>> -
>>>   	hdr.version = 6;
>>> +	/* default CID = 0, another if the CID flag is set */
>>> +	if (iphc1 & LOWPAN_IPHC_CID) {
>>> +		if (lowpan_fetch_skb(skb, &cid, sizeof(cid)))
>>> +			return -EINVAL;
>>> +	}
>>> +
>>>   	err = lowpan_iphc_tf_decompress(skb, &hdr,
>>>   					iphc0 & LOWPAN_IPHC_TF_MASK);
>>>   	if (err < 0)
>>> @@ -500,10 +563,18 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>>>   	}
>>>   	if (iphc1 & LOWPAN_IPHC_SAC) {
>>> -		/* Source address context based uncompression */
>>> +		spin_lock_bh(&lowpan_priv(dev)->iphc_sci.lock);
>>> +		ci = lowpan_ctx_by_id(dev, &lowpan_priv(dev)->iphc_sci,
>>> +				      LOWPAN_IPHC_CID_SCI(cid));
>>> +		if (!ci) {
>>> +			spin_unlock_bh(&lowpan_priv(dev)->iphc_sci.lock);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>>   		pr_debug("SAC bit is set. Handle context based source address.\n");
>>> -		err = uncompress_context_based_src_addr(skb, &hdr.saddr,
>>> -							iphc1 & LOWPAN_IPHC_SAM_MASK);
>>> +		err = uncompress_ctx_addr(skb, dev, ci, &hdr.saddr,
>>> +					  iphc1 & LOWPAN_IPHC_SAM_MASK, saddr);
>>> +		spin_unlock_bh(&lowpan_priv(dev)->iphc_sci.lock);
>>>   	} else {
>>>   		/* Source address uncompression */
>>>   		pr_debug("source address stateless compression\n");
>>> @@ -515,27 +586,54 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>>>   	if (err)
>>>   		return -EINVAL;
>>> -	/* check for Multicast Compression */
>>> -	if (iphc1 & LOWPAN_IPHC_M) {
>>> -		if (iphc1 & LOWPAN_IPHC_DAC) {
>>> -			pr_debug("dest: context-based mcast compression\n");
>>> -			/* TODO: implement this */
>>> -		} else {
>>> -			err = lowpan_uncompress_multicast_daddr(skb, &hdr.daddr,
>>> -								iphc1 & LOWPAN_IPHC_DAM_MASK);
>>> +	switch (iphc1 & (LOWPAN_IPHC_M | LOWPAN_IPHC_DAC)) {
>>> +	case LOWPAN_IPHC_M | LOWPAN_IPHC_DAC:
>>> +		spin_lock_bh(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +		ci = lowpan_ctx_by_id(dev, &lowpan_priv(dev)->iphc_mcast_dci,
>>> +				      LOWPAN_IPHC_CID_DCI(cid));
>>> +		if (!ci) {
>>> +			spin_unlock_bh(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +			return -EINVAL;
>>> +		}
>>> -			if (err)
>>> -				return -EINVAL;
>>> +		/* multicast with context */
>>> +		pr_debug("dest: context-based mcast compression\n");
>>> +		err = lowpan_uncompress_multicast_ctx_daddr(skb, ci,
>>> +							    &hdr.daddr,
>>> +							    iphc1 & LOWPAN_IPHC_DAM_MASK);
>>> +		spin_unlock_bh(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +		break;
>>> +	case LOWPAN_IPHC_M:
>>> +		/* multicast */
>>> +		err = lowpan_uncompress_multicast_daddr(skb, &hdr.daddr,
>>> +							iphc1 & LOWPAN_IPHC_DAM_MASK);
>>> +		break;
>>> +	case LOWPAN_IPHC_DAC:
>>> +		spin_lock_bh(&lowpan_priv(dev)->iphc_dci.lock);
>>> +		ci = lowpan_ctx_by_id(dev, &lowpan_priv(dev)->iphc_dci,
>>> +				      LOWPAN_IPHC_CID_DCI(cid));
>>> +		if (!ci) {
>>> +			spin_unlock_bh(&lowpan_priv(dev)->iphc_dci.lock);
>>> +			return -EINVAL;
>>>   		}
>>> -	} else {
>>> +
>>> +		/* Destination address context based uncompression */
>>> +		pr_debug("DAC bit is set. Handle context based destination address.\n");
>>> +		err = uncompress_ctx_addr(skb, dev, ci, &hdr.daddr,
>>> +					  iphc1 & LOWPAN_IPHC_DAM_MASK, daddr);
>>> +		spin_unlock_bh(&lowpan_priv(dev)->iphc_dci.lock);
>>> +		break;
>>> +	default:
>>>   		err = uncompress_addr(skb, dev, &hdr.daddr,
>>>   				      iphc1 & LOWPAN_IPHC_DAM_MASK, daddr);
>>>   		pr_debug("dest: stateless compression mode %d dest %pI6c\n",
>>>   			 iphc1 & LOWPAN_IPHC_DAM_MASK, &hdr.daddr);
>>> -		if (err)
>>> -			return -EINVAL;
>>> +		break;
>>>   	}
>>> +	if (err)
>>> +		return -EINVAL;
>>> +
>>>   	/* Next header data uncompression */
>>>   	if (iphc0 & LOWPAN_IPHC_NH) {
>>>   		err = lowpan_nhc_do_uncompression(skb, dev, &hdr);
>>> @@ -585,6 +683,60 @@ static const u8 lowpan_iphc_dam_to_sam_value[] = {
>>>   	[LOWPAN_IPHC_DAM_11] = LOWPAN_IPHC_SAM_11,
>>>   };
>>> +static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct in6_addr *ipaddr,
>>> +				   const struct lowpan_iphc_ctx *ctx,
>>> +				   const unsigned char *lladdr, bool sam)
>>> +{
>>> +	struct in6_addr tmp = {};
>>> +	u8 dam;
>>> +
>>> +	/* check for SAM/DAM = 11 */
>>> +	memcpy(&tmp.s6_addr[8], lladdr, 8);
>>> +	/* second bit-flip (Universe/Local)
>>> +	 * is done according RFC2464
>>> +	 */
>> Maybe put this comment on line. It should be short enough.
>>
> ok.
>
>>> +	tmp.s6_addr[8] ^= 0x02;
>>> +	/* context information are always used */
>>> +	ipv6_addr_prefix_copy(&tmp, &ctx->addr, ctx->prefix_len);
>>> +	if (ipv6_addr_equal(&tmp, ipaddr)) {
>>> +		dam = LOWPAN_IPHC_DAM_11;
>>> +		goto out;
>>> +	}
>>> +
>>> +	memset(&tmp, 0, sizeof(tmp));
>>> +	/* check for SAM/DAM = 01 */
>>> +	tmp.s6_addr[11] = 0xFF;
>>> +	tmp.s6_addr[12] = 0xFE;
>>> +	memcpy(&tmp.s6_addr[14], &ipaddr->s6_addr[14], 2);
>>> +	/* context information are always used */
>>> +	ipv6_addr_prefix_copy(&tmp, &ctx->addr, ctx->prefix_len);
>>> +	if (ipv6_addr_equal(&tmp, ipaddr)) {
>>> +		lowpan_push_hc_data(hc_ptr, &ipaddr->s6_addr[14], 2);
>>> +		dam = LOWPAN_IPHC_DAM_10;
>>> +		goto out;
>>> +	}
>>> +
>>> +	memset(&tmp, 0, sizeof(tmp));
>>> +	/* check for SAM/DAM = 10, should always match */
>>> +	memcpy(&tmp.s6_addr[8], &ipaddr->s6_addr[8], 8);
>>> +	/* context information are always used */
>>> +	ipv6_addr_prefix_copy(&tmp, &ctx->addr, ctx->prefix_len);
>>> +	if (ipv6_addr_equal(&tmp, ipaddr)) {
>>> +		lowpan_push_hc_data(hc_ptr, &ipaddr->s6_addr[8], 8);
>>> +		dam = LOWPAN_IPHC_DAM_01;
>>> +		goto out;
>>> +	}
>>> +
>>> +	WARN_ON_ONCE("context found but no address mode matched\n");
>>> +	return -EINVAL;
>>> +out:
>>> +
>>> +	if (sam)
>>> +		return lowpan_iphc_dam_to_sam_value[dam];
>>> +	else
>>> +		return dam;
>>> +}
>>> +
>>>   static u8 lowpan_compress_addr_64(u8 **hc_ptr, const struct in6_addr *ipaddr,
>>>   				  const unsigned char *lladdr, bool sam)
>>>   {
>>> @@ -708,6 +860,21 @@ static u8 lowpan_iphc_tf_compress(u8 **hc_ptr, const struct ipv6hdr *hdr)
>>>   	return val;
>>>   }
>>> +static u8 lowpan_iphc_mcast_ctx_addr_compress(u8 **hc_ptr,
>>> +					      const struct lowpan_iphc_ctx *ctx,
>>> +					      const struct in6_addr *ipaddr)
>>> +{
>>> +	u8 data[6];
>>> +
>>> +	/* flags/scope, reserved (RIID) */
>>> +	memcpy(data, &ipaddr->s6_addr[1], 2);
>>> +	/* group ID */
>>> +	memcpy(&data[1], &ipaddr->s6_addr[11], 4);
>>> +	lowpan_push_hc_data(hc_ptr, data, 6);
>>> +
>>> +	return LOWPAN_IPHC_DAM_00;
>>> +}
>>> +
>>>   static u8 lowpan_iphc_mcast_addr_compress(u8 **hc_ptr,
>>>   					  const struct in6_addr *ipaddr)
>>>   {
>>> @@ -742,10 +909,11 @@ static u8 lowpan_iphc_mcast_addr_compress(u8 **hc_ptr,
>>>   int lowpan_header_compress(struct sk_buff *skb, const struct net_device *dev,
>>>   			   const void *daddr, const void *saddr)
>>>   {
>>> -	u8 iphc0, iphc1, *hc_ptr;
>>> +	u8 iphc0, iphc1, *hc_ptr, cid = 0;
>>>   	struct ipv6hdr *hdr;
>>>   	u8 head[LOWPAN_IPHC_MAX_HC_BUF_LEN] = {};
>>> -	int ret, addr_type;
>>> +	struct lowpan_iphc_ctx *dci, *sci, dci_entry, sci_entry;
>>> +	int ret, ipv6_daddr_type, ipv6_saddr_type;
>>>   	if (skb->protocol != htons(ETH_P_IPV6))
>>>   		return -EINVAL;
>>> @@ -769,14 +937,47 @@ int lowpan_header_compress(struct sk_buff *skb, const struct net_device *dev,
>>>   	iphc0 = LOWPAN_DISPATCH_IPHC;
>>>   	iphc1 = 0;
>>> -	/* TODO: context lookup */
>>> -
>>>   	raw_dump_inline(__func__, "saddr", saddr, EUI64_ADDR_LEN);
>>>   	raw_dump_inline(__func__, "daddr", daddr, EUI64_ADDR_LEN);
>>>   	raw_dump_table(__func__, "sending raw skb network uncompressed packet",
>>>   		       skb->data, skb->len);
>>> +	ipv6_daddr_type = ipv6_addr_type(&hdr->daddr);
>>> +	if (ipv6_daddr_type & IPV6_ADDR_MULTICAST) {
>>> +		spin_lock_bh(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +		dci = lowpan_ctx_by_addr(dev, &lowpan_priv(dev)->iphc_mcast_dci,
>>> +					 &hdr->daddr);
>>> +		if (dci) {
>>> +			memcpy(&dci_entry, dci, sizeof(*dci));
>>> +			cid |= dci->id;
>>> +		}
>>> +		spin_unlock_bh(&lowpan_priv(dev)->iphc_mcast_dci.lock);
>>> +	} else {
>>> +		spin_lock_bh(&lowpan_priv(dev)->iphc_dci.lock);
>>> +		dci = lowpan_ctx_by_addr(dev, &lowpan_priv(dev)->iphc_dci,
>>> +					 &hdr->daddr);
>>> +		if (dci) {
>>> +			memcpy(&dci_entry, dci, sizeof(*dci));
>>> +			cid |= dci->id;
>>> +		}
>>> +		spin_unlock_bh(&lowpan_priv(dev)->iphc_dci.lock);
>>> +	}
>>> +
>>> +	spin_lock_bh(&lowpan_priv(dev)->iphc_sci.lock);
>>> +	sci = lowpan_ctx_by_addr(dev, &lowpan_priv(dev)->iphc_sci,
>>> +				 &hdr->saddr);
>>> +	if (sci) {
>>> +		memcpy(&sci_entry, sci, sizeof(*sci));
>>> +		cid |= (sci->id << 4);
>>> +	}
>>> +	spin_unlock_bh(&lowpan_priv(dev)->iphc_sci.lock);
>>> +
>>> +	if (sci || dci) {
>>> +		iphc1 |= LOWPAN_IPHC_CID;
>>> +		lowpan_push_hc_data(&hc_ptr, &cid, sizeof(cid));
>>> +	}
>>> +
>>>   	/* Traffic Class, Flow Label compression */
>>>   	iphc0 |= lowpan_iphc_tf_compress(&hc_ptr, hdr);
>>> @@ -813,39 +1014,63 @@ int lowpan_header_compress(struct sk_buff *skb, const struct net_device *dev,
>>>   				    sizeof(hdr->hop_limit));
>>>   	}
>>> -	addr_type = ipv6_addr_type(&hdr->saddr);
>>> +	ipv6_saddr_type = ipv6_addr_type(&hdr->saddr);
>>>   	/* source address compression */
>>> -	if (addr_type == IPV6_ADDR_ANY) {
>>> +	if (ipv6_saddr_type == IPV6_ADDR_ANY) {
>>>   		pr_debug("source address is unspecified, setting SAC\n");
>>>   		iphc1 |= LOWPAN_IPHC_SAC;
>>>   	} else {
>>> -		if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>> -			iphc1 |= lowpan_compress_addr_64(&hc_ptr, &hdr->saddr,
>>> -							 saddr, true);
>>> -			pr_debug("source address unicast link-local %pI6c iphc1 0x%02x\n",
>>> -				 &hdr->saddr, iphc1);
>>> +		if (sci) {
>>> +			iphc1 |= lowpan_compress_ctx_addr(&hc_ptr, &hdr->saddr,
>>> +							  &sci_entry, saddr,
>>> +							  true);
>>> +			iphc1 |= LOWPAN_IPHC_SAC;
>>>   		} else {
>>> -			pr_debug("send the full source address\n");
>>> -			lowpan_push_hc_data(&hc_ptr, hdr->saddr.s6_addr, 16);
>>> +			if (ipv6_saddr_type & IPV6_ADDR_LINKLOCAL) {
>>> +				iphc1 |= lowpan_compress_addr_64(&hc_ptr,
>>> +								 &hdr->saddr,
>>> +								 saddr, true);
>>> +				pr_debug("source address unicast link-local %pI6c iphc1 0x%02x\n",
>>> +					 &hdr->saddr, iphc1);
>>> +			} else {
>>> +				pr_debug("send the full source address\n");
>>> +				lowpan_push_hc_data(&hc_ptr,
>>> +						    hdr->saddr.s6_addr, 16);
>>> +			}
>>>   		}
>>>   	}
>>> -	addr_type = ipv6_addr_type(&hdr->daddr);
>>>   	/* destination address compression */
>>> -	if (addr_type & IPV6_ADDR_MULTICAST) {
>>> +	if (ipv6_daddr_type & IPV6_ADDR_MULTICAST) {
>>>   		pr_debug("destination address is multicast: ");
>>> -		iphc1 |= LOWPAN_IPHC_M;
>>> -		iphc1 |= lowpan_iphc_mcast_addr_compress(&hc_ptr, &hdr->daddr);
>>> +		if (dci) {
>>> +			iphc1 |= lowpan_iphc_mcast_ctx_addr_compress(&hc_ptr,
>>> +								     &dci_entry,
>>> +								     &hdr->daddr);
>>> +		} else {
>>> +			iphc1 |= LOWPAN_IPHC_M;
>>> +			iphc1 |= lowpan_iphc_mcast_addr_compress(&hc_ptr,
>>> +								 &hdr->daddr);
>>> +		}
>>>   	} else {
>>> -		if (addr_type & IPV6_ADDR_LINKLOCAL) {
>>> -			/* TODO: context lookup */
>>> -			iphc1 |= lowpan_compress_addr_64(&hc_ptr, &hdr->daddr,
>>> -							 daddr, false);
>>> -			pr_debug("dest address unicast link-local %pI6c "
>>> -				 "iphc1 0x%02x\n", &hdr->daddr, iphc1);
>>> +		if (dci) {
>>> +			iphc1 |= lowpan_compress_ctx_addr(&hc_ptr, &hdr->daddr,
>>> +							  &dci_entry, daddr,
>>> +							  false);
>>> +			iphc1 |= LOWPAN_IPHC_DAC;
>>>   		} else {
>>> -			pr_debug("dest address unicast %pI6c\n", &hdr->daddr);
>>> -			lowpan_push_hc_data(&hc_ptr, hdr->daddr.s6_addr, 16);
>>> +			if (ipv6_daddr_type & IPV6_ADDR_LINKLOCAL) {
>>> +				iphc1 |= lowpan_compress_addr_64(&hc_ptr,
>>> +								 &hdr->daddr,
>>> +								 daddr, false);
>>> +				pr_debug("dest address unicast link-local %pI6c iphc1 0x%02x\n",
>>> +					 &hdr->daddr, iphc1);
>>> +			} else {
>>> +				pr_debug("dest address unicast %pI6c\n",
>>> +					 &hdr->daddr);
>>> +				lowpan_push_hc_data(&hc_ptr,
>>> +						    hdr->daddr.s6_addr, 16);
>>> +			}
>>>   		}
>>>   	}
>>> @@ -871,3 +1096,143 @@ int lowpan_header_compress(struct sk_buff *skb, const struct net_device *dev,
>>>   	return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(lowpan_header_compress);
>>> +
>>> +static bool lowpan_iphc_ctx_valid_prefix(const struct lowpan_iphc_ctx *ctx)
>>> +{
>>> +	/* prefix which are smaller than 64 bits are not valid, users
>>> +	 * may mean than a prefix which is filled with zero until 64th bit.
>>> +	 * Refere rfc6282 "Any remaining bits are zero." The remaining bits
>>> +	 * in this case where are the prefix(< 64) ends until IID starts.
>>> +	 */
>>> +	if (ctx->prefix_len < 64)
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>>> +
> I will remove this check, the prefix_len should be any length, from 0
> until 128. We need to care about the zero bits if prefix_len < 64 during
> lookup.
>
> I think a "valid" test would be to check on multicast address scope
> which should not a valid prefix for dci/sci table. There might be more
> addresses which should not valid here. At the moment I would say a
> multicast address should be invalid. I will add this as well.
>
>>> +static bool
>>> +lowpan_iphc_mcast_ctx_valid_prefix(const struct lowpan_iphc_ctx *ctx)
>>> +{
>>> +	/* network prefix for multicast is at maximum 64 bits long */
>>> +	if (ctx->prefix_len > 64)
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +static int lowpan_iphc_ctx_update(struct lowpan_iphc_ctx *table,
>>> +				  const struct lowpan_iphc_ctx *ctx)
>>> +{
>>> +	int ret = 0, i;
>>> +
>>> +	if (ctx->enabled) {
>>> +		for (i = 0; i < LOWPAN_IPHC_CI_TABLE_SIZE; i++) {
>>> +			if (ctx->prefix_len != table[i].prefix_len)
>>> +				continue;
>>> +
>>> +			if (ipv6_prefix_equal(&ctx->addr, &table[i].addr,
>>> +					      ctx->prefix_len)) {
>>> +				if (table[i].enabled) {
>>> +					ret = -EEXIST;
>>> +					goto out;
>>> +				}
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	memcpy(&table[ctx->id], ctx, sizeof(*ctx));
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>> In what cases should the update succeed? I'm wondering about some corner
>> cases here:
>>
>> o If the new ctx is disabled it would update the table on its cid no matter
>> what.
>> o If the new ctx is enabled we would update it only if we do not find the
>> same prefix in an enabled state already.
>>
>> To put it differently, the only case we do  not update the table with the
>> context is if it already exists and is enabled. This is how you want it?
>>
> yes. If we don't check if it's enabled (inside the table), then we could
> not simple copy one line (by doing cat and echo) with a context which
> is disabled, then simple replace the disabled(0) to enabled(1). Then it
> would fail because the same prefix is already there, but when it's
> disabled then we ignore if the prefix is already exists. It will not
> used then anyway.

ok

regards
Stefan Schmidt