Date:	Fri, 2 May 2008 22:23:03 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
Cc:	acme@...hat.com, dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] [QPOLICY]: cmsg header parsing fixes

| dccp_msghdr_parse can now parse arbitrary DCCP_SCM_* fields and put them
| directly into skb. Now the function also sets deault values and ignores
| unknown fields. Moved clearing of used skb fields to dccp_qpolicy_pop.
| 
The clearing of the skb-fields is a very good idea and I would like to
add this to the patch. With the other parts I am having some difficulties;
there were reasons why it was done this way - comments below.

| --- a/net/dccp/proto.c
| +++ b/net/dccp/proto.c
| @@ -719,13 +722,21 @@ static int dccp_msghdr_parse(struct msghdr *msg, __u32 **priority)
|  			continue;
|  
|  		switch (cmsg->cmsg_type) {
| +		/*
| +		 * Assign the (opaque) qpolicy priority value to skb->priority
| +		 *
| +		 * We are overloading this skb field for use with the qpolicy
| +		 * subystem. The skb->priority is normally used for the
| +		 * SO_PRIORITY option, which is initialised from sk_priority
| +		 * Since the assignment of sk_priority to skb->priority happens
| +		 * later (on layer 3), we overload this field for use with
| +		 * queueing priorities as long as the skb is on layer 4.
| +		 */
|  		case DCCP_SCM_PRIORITY:
|  			if (cmsg->cmsg_len != CMSG_LEN(sizeof(__u32)))
|  				return -EINVAL;
| -			*priority = (__u32 *)CMSG_DATA(cmsg);
| +			skb->priority = *(__u32 *)CMSG_DATA(cmsg);
|  			break;
| -		default:
| -			return -EINVAL;
|  		}
|  	}
|  	return 0;
It should thrown an (EINVAL) error if the user supplied a wrong parameter - this function only 
checks for socket level SOL_DCCP and so if a wrong SCM_xxx is passed, it is an error.

With regard to the typecast, I found that such typecasts sometimes lead to alignment errors, 
i.e. would require to use get_unaligned() (compare e.g.	ccid3_hc_tx_parse_options() where this
happened before using get_unaligned()).

| @@ -739,15 +750,11 @@ int dccp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
|  	const int noblock = flags & MSG_DONTWAIT;
|  	struct sk_buff *skb;
|  	int rc, size;
| -	__u32 *pprio = NULL;
|  	long timeo;
|  
|  	if (len > dp->dccps_mss_cache)
|  		return -EMSGSIZE;
|  
| -	if (dccp_msghdr_parse(msg, &pprio))
| -		return -EINVAL;
| -
|  	lock_sock(sk);
|  
|  	if (dccp_qpolicy_full(sk)) {
The reason that it was placed here is that if the user supplies a wrong
parameter, the socket is first locked, then an skb is allocated, and
only at this late stage it is discovered that dccp_msghdr_parse() found
an invalid value.

	/* break */

I can see what you are trying to do, and there is a good idea in it: we
could get rid of the requirement to use skb->fields by
 
 1. declaring a 

    struct dccp_qpolicy_args {
	    	__s8	priority;	/* -127 .. 128 as in older  version */
		struct timeval timeout;
		/* ... other fields, or maybe even arranged as union */
    };

 2. in dccp_sendmsg(), call
 
    struct dccp_qpolicy_args qp_args;

    // ...
    if (dccp_msghdr_parse(msg, &pprio, &qp_args))
		return -EINVAL;

 3. dccp_msghdr_parse() fills in default values as in your approach here
    (could use a simple memset() to start this)

 4. then at the bottom of dccp_sendmsg(), the struct is passed on to 

    dccp_qpolicy_push(sk, skb, &qp_args);


Of course, it means a little more work since all xxx_push() routines
need to have a qpolicy_args argument. But the advantage that I see is
that this gives the extensibility (and that reoccurred in your comments)
plus it is clearer (no hidden overloading of skb fields).

Maybe I missed something?


The University of Aberdeen is a charity registered in Scotland, No SC013683.

--
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