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

Dnia Tuesday 06 of May 2008, Gerrit Renker napisał:
> | > | -		default:
> | > | -			return -EINVAL;
> | >
> | > 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?
>
> In the online version of your patch (subtree `qpolicy' of the test tree)
> I have added them back in for now.
>
> By taking them out, you are robbing yourself of the possibility to
> distinguish between truly invalid arguments ("EINVALs") and
> incompatibilities.
>
But do we need such distinction? If kernel cannot understand part of cmsg data 
we should just ignore this part. The application will work, maybe slightly 
less effective than the author intended but it's better than returning error 
and making it completely useless. This could be called best-effort approach. 
However, we could return error on unknown or invalid data provided 
applications can detect which parameters are supported prior to packet 
submission.

To summarise I see two options here:
1. Provide a way for applications to ask at runtime which DCCP_SCM_xxx options 
are supported by running kernel. And always return EINVAL on unsupported 
data.
2. Allow ignoring unknown data, it doesn't have to be the default. This could 
take a form of setsockopt(IGNORE_UNKNOWN_CMSG_DATA) call.

Both ways the default would be as you propose but applications would be able 
to work around older kernel version. What do you think about it?

> If it is really for an older kernel version, one can use something like
> the statement in net/dccp/proto.c
>
>   DCCP_WARN("sockopt(PACKET_SIZE) is deprecated: fix your app\n");
>
The warning could be useful for application developers. I'm not sure if it 
makes sense to "display" it to users. But that's a minor issue.

> The main point of the perhaps a bit prolonged discussion is to avoid too
> many deprecated warnings, i.e. I'd prefer that the API does not change
> too often.
>
> On the other hand, making these changes is exactly what the test tree is
> for - so that change/revert cycles and deprecated-API warnings  need not be
> done later in mainline.
>
Sure it's better to minimize the amount of API changes in mainline. But I 
prefer to prepare for changes rather than avoid them.

> | > 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?
>
> Not sure about this: the question is how memory is accessed in the typecast
>
> 	skb->priority = *(__u32 *)CMSG_DATA(cmsg);
>
> and how is this access aligned? Doing a similar typecast for DCCP
> options (Timestamp, Timestamp Echo, Elapsed Time, CCID-3's Loss Event
> and Receive Rate) caused an alignment exception on Sparc64, since
> (half)word accesses need to be aligned on 2/4 byte boundaries.
>
> I tested the above and it actually worked. Since I can't say whether this
> was by accident or not, so it does seem safer to use get_unaligned().
>
I'm no expert on data alignment so I have to trust you here. Change as you 
think is correct.

> | 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;
> | )?
>
> That is the point I missed: it is not necessary to equip all xxx_push()
> routines with an extra argument, this can be done by the
> dccp_qpolicy_push() routine.
>
Could be, in fact I thought about moving code from dccp_msghdr_parse() to 
dccp_qpolicy_push(). But on the other hand cmsg data doesn't have to be only 
about qpolicies. I mean in theory because currently I cannot show a usecase 
for non-qpolicy specific cmsg data. For me the code could either be as it is 
or could be moved to dccp_qpolicy_push(). Both options seem to be ok.

> The error in the above was to assume that qpolicy_args makes it no
> longer necessary to use skb fields, which is not true.
>
Right, it's not :-(

> I (...) am questioning whether this is not really over-designed.
> 
I wonder if you could rephrase this sentence possibly using a bit simpler 
English.
-- 
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