On Tue, Feb 28, 2017 at 2:46 PM, Alexander Duyck
<alexander.duyck@xxxxxxxxx> wrote:
> On Tue, Feb 28, 2017 at 2:32 AM, Davide Caratti <dcaratti@xxxxxxxxxx> wrote:
>> sctp_compute_checksum requires crc32c symbol (provided by libcrc32c), so
>> it can't be used in net core. Like it has been done previously with other
>> symbols (e.g. ipv6_dst_lookup), introduce a stub struct skb_checksum_ops
>> to allow computation of SCTP checksum in net core after sctp.ko (and thus
>> libcrc32c) has been loaded.
>
> At a minimum the name really needs to change. SCTP does not do
> checksums. It does a CRC, and a CRC is a very different thing. The
> fact that somebody decided that offloading a CRC could use the same
> framework is very unfortunate, and your patch descriptions in this
> whole set are calling out a CRC as checksums which it is not.
>
> I don't want to see anything "checksum" or "csum" related in the
> naming when it comes to dealing with SCTP unless we absolutely have to
> have it. So any function names or structures with sctp in the name
> should call out "crc32" or "crc", please don't use checksum.
>
Alexander,
I agree that internal functions to sctp should not refer to checksum,
but I think we need to take care to be consistent with any external
API (even if somebody made a mistake defining it this way :-) ). As
you know the checksum interface must be very precisely defined, there
is no leeway for ambiguity. Many places in the stack use csum and
CHECKSUM_* to refer to the API not the actual algorithm, others don't
(e.g. CHECKSUM_UNNECESSARY can apply to SCTP checksum,
CHECKSUM_COMPLETE must be an Internet checksum).
For instance, in that light skb_sctp_csum_help is appropriately named
I think because this is being called from skb_csum_help and refers to
the interface to resolve a checksum.
Tom
>> Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
>> Signed-off-by: Davide Caratti <dcaratti@xxxxxxxxxx>
>> ---
>> include/linux/skbuff.h | 2 ++
>> net/core/skbuff.c | 20 ++++++++++++++++++++
>> net/sctp/offload.c | 7 +++++++
>> 3 files changed, 29 insertions(+)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 69ccd26..cab9a32 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -3125,6 +3125,8 @@ struct skb_checksum_ops {
>> __wsum (*combine)(__wsum csum, __wsum csum2, int offset, int len);
>> };
>>
>> +extern const struct skb_checksum_ops *sctp_csum_stub __read_mostly;
>> +
>> __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
>> __wsum csum, const struct skb_checksum_ops *ops);
>> __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index f355795..64fd8fd 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -2242,6 +2242,26 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
>> }
>> EXPORT_SYMBOL(skb_copy_and_csum_bits);
>>
>> +static __wsum warn_sctp_csum_update(const void *buff, int len, __wsum sum)
>> +{
>> + net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
>> + return 0;
>> +}
>> +
>> +static __wsum warn_sctp_csum_combine(__wsum csum, __wsum csum2,
>> + int offset, int len)
>> +{
>> + net_warn_ratelimited("attempt to compute crc32c without sctp.ko\n");
>> + return 0;
>> +}
>> +
>> +const struct skb_checksum_ops *sctp_csum_stub __read_mostly =
>> + &(struct skb_checksum_ops) {
>> + .update = warn_sctp_csum_update,
>> + .combine = warn_sctp_csum_combine,
>> +};
>> +EXPORT_SYMBOL(sctp_csum_stub);
>> +
>> /**
>> * skb_zerocopy_headlen - Calculate headroom needed for skb_zerocopy()
>> * @from: source buffer
>> diff --git a/net/sctp/offload.c b/net/sctp/offload.c
>> index 4f5a2b5..e9c3db0 100644
>> --- a/net/sctp/offload.c
>> +++ b/net/sctp/offload.c
>> @@ -98,6 +98,12 @@ static const struct net_offload sctp6_offload = {
>> },
>> };
>>
>> +static const struct skb_checksum_ops *sctp_csum_ops __read_mostly =
>> + &(struct skb_checksum_ops) {
>> + .update = sctp_csum_update,
>> + .combine = sctp_csum_combine,
>> +};
>> +
>> int __init sctp_offload_init(void)
>> {
>> int ret;
>> @@ -110,6 +116,7 @@ int __init sctp_offload_init(void)
>> if (ret)
>> goto ipv4;
>>
>> + sctp_csum_stub = sctp_csum_ops;
>> return ret;
>>
>> ipv4:
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
