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. -- Jarod Wilson jarod@xxxxxxxxxx -- 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
