Message ID | 1322317612-7770-1-git-send-email-lindner_marek@yahoo.de |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Marek Lindner <lindner_marek@yahoo.de> Date: Sat, 26 Nov 2011 22:26:42 +0800 > the following 10 patches constitute the first batch I'd like to get the pulled > into net-next-2.6/3.3. They're mostly uncritical fixes around the recently > introduced tt code, some code refactoring, the kstrto update and the range > check fix reported by Thomas Jarosch. Pulled, thanks. Some things to look into: + if (unlikely(skb_headlen(skb) < + sizeof(struct tt_query_packet) + + tt_len)) This isn't formatted correctly, all the leading edges should line up to the openning parenthesis of the unlikely: + if (unlikely(skb_headlen(skb) < + sizeof(struct tt_query_packet) + + tt_len)) Next, there is a lot of linearization done by the stack, but really the thing to do is to make sure that the part you want to look at is linear. You do this using pskb_may_pull() right before you want to look at some headers. It makes sure that, for the length given, that many bytes are linear at the head of the skb. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello David, On Sat, Nov 26, 2011 at 02:41:22 -0500, David Miller wrote: [CUT] > Some things to look into: > > + if (unlikely(skb_headlen(skb) < > + sizeof(struct tt_query_packet) + > + tt_len)) > > This isn't formatted correctly, all the leading edges should line > up to the openning parenthesis of the unlikely: > > + if (unlikely(skb_headlen(skb) < > + sizeof(struct tt_query_packet) + > + tt_len)) > Thank you for reporting this issue. We have already prepared a patch which is going to be sent within the next batch. > Next, there is a lot of linearization done by the stack, but really the > thing to do is to make sure that the part you want to look at is > linear. > > You do this using pskb_may_pull() right before you want to look at some > headers. It makes sure that, for the length given, that many bytes are > linear at the head of the skb. > For this issue, we had some problem to understand it. First of all I think you are referring to patch 08/10, in which I moved a skb_linearise() operation. To be sure it is really needed, I backtracked the code flow and noted down any eventual psbk_may_pull() (or any other linearisation operation). The result is: => in batman_skb_recv() - pskb_may_pull(2) => in recv_tt_query() - pskb_may_pull(sizeof(header)) - skb_linearise() Actually it seems we haven't any useless linearisation. Would you mind explain us where you actually found the problem, please? It might also be that I misunderstood your advice. Thank you. Best Regards,
From: Antonio Quartulli <ordex@autistici.org> Date: Fri, 2 Dec 2011 18:12:16 +0100 > First of all I think you are referring to patch 08/10, in which I moved a > skb_linearise() operation. > > To be sure it is really needed, I backtracked the code flow and noted down any > eventual psbk_may_pull() (or any other linearisation operation). The result is: > > => in batman_skb_recv() > - pskb_may_pull(2) > => in recv_tt_query() > - pskb_may_pull(sizeof(header)) > - skb_linearise() > > Actually it seems we haven't any useless linearisation. > Would you mind explain us where you actually found the problem, please? > > It might also be that I misunderstood your advice. You only need to call pskb_may_pull() on the parts of the packet you want to access directly to parse headers etc. If you use that interface properly, you never need to linearize, ever. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 02, 2011 at 12:57:25 -0500, David Miller wrote: > From: Antonio Quartulli <ordex@autistici.org> > Date: Fri, 2 Dec 2011 18:12:16 +0100 > > > First of all I think you are referring to patch 08/10, in which I moved a > > skb_linearise() operation. > > > > To be sure it is really needed, I backtracked the code flow and noted down any > > eventual psbk_may_pull() (or any other linearisation operation). The result is: > > > > => in batman_skb_recv() > > - pskb_may_pull(2) > > => in recv_tt_query() > > - pskb_may_pull(sizeof(header)) > > - skb_linearise() > > > > Actually it seems we haven't any useless linearisation. > > Would you mind explain us where you actually found the problem, please? > > > > It might also be that I misunderstood your advice. > > You only need to call pskb_may_pull() on the parts of the packet you want to > access directly to parse headers etc. > > If you use that interface properly, you never need to linearize, ever. Sorry for being too generic: we actually invoke skb_linearise() because we want to access the whole skb. We first call pskb_may_pull() to pull the header only and then, under certain conditions, we linearise the whole skb to access it all. Should I use pskb_may_pull() even in this case? Sorry for stealing you so much time on this issue, but I would like to fully understand it in order to avoid any further mistake. Thank you again. Best Regards,
From: Antonio Quartulli <ordex@autistici.org> Date: Sat, 3 Dec 2011 02:55:29 +0100 > We first call pskb_may_pull() to pull the header only and then, under certain > conditions, we linearise the whole skb to access it all. Should I use > pskb_may_pull() even in this case? Why would you need to access the whole thing? There are only two types of possible accesses: 1) Header parsing --> use pskb_may_pull() as needed. 2) Copying the data to some other location, such as a user buffer. Use skb_copy_datagram_iovec or similar which handle fragmented SKBs just fine. You should handle fragmented SKBs as much as possible, because linearization is expensive and often amounts to a memory allocation plus a copy if you linearize the whole thing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html