Jarod Wilson wrote:
On Fri, May 21, 2010 at 11:40:34AM +0800, Ang Way Chuang wrote:
Hi Jarod,
Thanks for the review. My answers are inlined.
Jarod Wilson wrote:
On Thu, May 06, 2010 at 02:52:22PM -0000, Ang Way Chuang wrote:
ULE (Unidirectional Lightweight Encapsulation RFC 4326)
decapsulation code has a bug that incorrectly treats ULE SNDU
packed into the remaining 2 or 3 bytes of a MPEG2-TS frame as
having invalid pointer field on the subsequent MPEG2-TS frame.
This patch was generated and tested against v2.6.34-rc6. I
suspect that this bug was introduced in kernel version 2.6.15,
but had not verified it.
Care has been taken not to introduce more bug by fixing this bug, but
please scrutinize the code because I always produces buggy code.
...
@@ -534,6 +535,7 @@ static void dvb_net_ule( struct net_device *dev, const u8 *buf, size_t buf_len )
from_where += 2;
}
+ priv->ule_sndu_remain = priv->ule_sndu_len + 2;
/*
* State of current TS:
* ts_remain (remaining bytes in the current TS cell)
Is this *always* true? Your description says "...the remaining 2 or 3
bytes", indicating this could sometimes need to be +3. Is +0 also a
possibility?
Not sure whether I understand your question correctly. Here is my
attempt to answer your question. The encapsulation format always
mandate that at least of 2 bytes of ULE SNDU (the LENGTH field) must
be present within a MPEG2-TS frame. So, if only 1 byte of the ULE
SNDU get packed into the remaining MPEG2-TS frame, then it is
invalid. Of course, there is no issue regarding 0 byte as that would
be the case of filling MPEG2-TS frame up to its boundary. New ULE
SNDU will have to packed into the next MPEG2-TS frame in that case.
Now the problem with existing code is the interpretation of
remainder length when 2 or 3 bytes of ULE SNDU are packed into the
remainder of MPEG2-TS frame. In the 2 bytes case, only the LENGTH
field is available while in the case 3 bytes, only the 1st octet of
the 2-octets TYPE field and the LENGTH field are available. The
ule_sndu_remain should carry the value of length of ULE SNDU
following the the TYPE field. Technically, this would mean that
remainder byte of ULE SNDU that need to be received is going to be:
Value(LENGTH) + 2 (We owe 2 bytes of TYPE field here) if only 2
bytes of ULE SNDU is packed (as in the case of case 0: at line 550).
This is addressed by adding the priv->ule_sndu_remain =
priv->ule_sndu_len + 2;
Value(LENGTH) + 1 (We owe 1 byte of TYPE field here) if 3 bytes of
ULE SNDU is packed (as in the case of case 1: at 545). This is
addressed by adding priv->ule_sndu_remain--;
If complete ULE header (>= 4 bytes) is available:
priv->ule_sndu_remain = priv->ule_sndu_len; at line 582 takes care
of the rest and it works just fine in the existing code.
Due to the wrong interpretation of remaining length of ULE SNDU when
2 or 3 bytes of ULE SNDU are packed into a MPEG2-TS frame, the
subsequent checking of payload pointer (line 455) always fails
leading to unnecessary packet drops.
Looking back at the fix after a few months, I had trouble
understanding how these few lines fixed the problem at first glance.
Yeah, my question was whether or not the +2 would account for both the +2
bytes and +3 bytes situations, and it seems that's handled appropriately
by the ts_remain switch. Thank you for the detailed explanation.
If you'd alter that nested check for freeing the skb and give it a quick
test, I'm happy to throw an acked-by or reviewed-by on a followup
submission.
Got it. Thank you. I shall get that patch to you next week because I'm not in the lab now.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
