Message-Id: <200805030011.53490.tomasz@grobelny.oswiecenia.net>
Date:	Sat, 3 May 2008 00:11:53 +0200
From:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
To:	Gerrit Renker <gerrit@....abdn.ac.uk>, acme@...hat.com
Cc:	dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] [QPOLICY]: cmsg header parsing fixes

Dnia Friday 02 of May 2008, Gerrit Renker napisał:
> | --- 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.
>
I deleted those two lines on purpose. If the kernel cannot understand given 
DCCP_SCM_xxx then the application may have been written for newer kernel. For 
example if application will specify DCCP_SCM_TIMEOUT and kernel does not 
understand it the application can still run without problems. It just won't 
be that effective.

Maybe there are cases when all DCCP_SCM_xxx must be understood by kernel. For 
such cases we could have something like setsockopt(MUST_UNDERSTAND_CMSG), 
would that be ok?

> 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()).
>
The CMSG_DATA() macro seems to take care of alignment, doesn't it?

> | @@ -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.
>
While it is possible that data passed from userspace will be incorrect it will 
happen very rarely. I mean in correctly written applications the data passed 
to dccp_sendmsg will be 100% correct. Otherwise the user wouldn't use the 
application, would (s)he? There is no need to optimize code for error 
conditions (unless it may lead to DoS, but I doubt it).

> 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?
>
I'm not sure if it's not a bit over-designed approach. But apart from that I 
have nothing against it.

One question: what should xxx_push() do with this additional argument? Store 
it as whole (note that we don't have room in skb->cb) or rewrite individual 
fields of dccp_qpolicy_args to fields of skb (like:
skb->priority = qp_args->priority;
)?
-- 
Regards,
Tomasz Grobelny
--
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