| Message ID | 20211201000215.1134831-2-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework parsing of HCI events | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Tue, 30 Nov 2021 16:02:01 -0800 Luiz Augusto von Dentz wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Like skb_pull but returns the original data pointer before pulling the > data after performing a check against sbk->len. > > This allows to change code that does "struct foo *p = (void *)skb->data;" > which is hard to audit and error prone, to: > > p = skb_pull_data(skb, sizeof(*p)); > if (!p) > return; > > Which is both safer and cleaner. It doesn't take a data pointer, so not really analogous to skb_put_data() and friends which come to mind. But I have no better naming suggestions. You will need to respin, tho, if you want us to apply these directly, the patches as posted don't apply to either netdev tree.
Hi Jakub, On Tue, Nov 30, 2021 at 5:11 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 30 Nov 2021 16:02:01 -0800 Luiz Augusto von Dentz wrote: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > Like skb_pull but returns the original data pointer before pulling the > > data after performing a check against sbk->len. > > > > This allows to change code that does "struct foo *p = (void *)skb->data;" > > which is hard to audit and error prone, to: > > > > p = skb_pull_data(skb, sizeof(*p)); > > if (!p) > > return; > > > > Which is both safer and cleaner. > > It doesn't take a data pointer, so not really analogous to > skb_put_data() and friends which come to mind. But I have > no better naming suggestions. You will need to respin, tho, > if you want us to apply these directly, the patches as posted > don't apply to either netdev tree. I cross posted it to net-dev just in case you guys had some strong opinions on introducing such a function, it was in fact suggested by Dan but I also didn't find a better name so I went with it, if you guys prefer we can merge it in bluetooth-next first as usual.
On Tue, 30 Nov 2021 18:16:02 -0800 Luiz Augusto von Dentz wrote: > > It doesn't take a data pointer, so not really analogous to > > skb_put_data() and friends which come to mind. But I have > > no better naming suggestions. You will need to respin, tho, > > if you want us to apply these directly, the patches as posted > > don't apply to either netdev tree. > > I cross posted it to net-dev just in case you guys had some strong > opinions on introducing such a function, Someone else still may, I don't :) > it was in fact suggested by Dan but I also didn't find a better name > so I went with it, if you guys prefer we can merge it in > bluetooth-next first as usual. Going via bluetooth-next sounds good!
Thanks for following up on this! I had forgotten about it. I'm really happy this is going forward. regards, dan carpenter
Hi Jakub, >>> It doesn't take a data pointer, so not really analogous to >>> skb_put_data() and friends which come to mind. But I have >>> no better naming suggestions. You will need to respin, tho, >>> if you want us to apply these directly, the patches as posted >>> don't apply to either netdev tree. >> >> I cross posted it to net-dev just in case you guys had some strong >> opinions on introducing such a function, > > Someone else still may, I don't :) > >> it was in fact suggested by Dan but I also didn't find a better name >> so I went with it, if you guys prefer we can merge it in >> bluetooth-next first as usual. > > Going via bluetooth-next sounds good! if you are ok with this going via bluetooth-next, then I need some sort of ACK from you or Dave. Regards Marcel
On Wed, 1 Dec 2021 08:22:51 +0100 Marcel Holtmann wrote: > >> I cross posted it to net-dev just in case you guys had some strong > >> opinions on introducing such a function, > > > > Someone else still may, I don't :) > > > >> it was in fact suggested by Dan but I also didn't find a better name > >> so I went with it, if you guys prefer we can merge it in > >> bluetooth-next first as usual. > > > > Going via bluetooth-next sounds good! > > if you are ok with this going via bluetooth-next, then I need some sort > of ACK from you or Dave. Acked-by: Jakub Kicinski <kuba@kernel.org>
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index eba256af64a5..877dda38684a 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2373,6 +2373,8 @@ static inline void *skb_pull_inline(struct sk_buff *skb, unsigned int len) return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len); } +void *skb_pull_data(struct sk_buff *skb, size_t len); + void *__pskb_pull_tail(struct sk_buff *skb, int delta); static inline void *__pskb_pull(struct sk_buff *skb, unsigned int len) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index a33247fdb8f5..0b19833ffbce 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2023,6 +2023,29 @@ void *skb_pull(struct sk_buff *skb, unsigned int len) } EXPORT_SYMBOL(skb_pull); +/** + * skb_pull_data - remove data from the start of a buffer returning its + * original position. + * @skb: buffer to use + * @len: amount of data to remove + * + * This function removes data from the start of a buffer, returning + * the memory to the headroom. A pointer to the original data in the buffer + * is returned after checking if there is enough data to pull. Once the + * data has been pulled future pushes will overwrite the old data. + */ +void *skb_pull_data(struct sk_buff *skb, size_t len) +{ + void *data = skb->data; + + if (skb->len < len) + return NULL; + + skb_pull(skb, len); + + return data; +} + /** * skb_trim - remove end from a buffer * @skb: buffer to alter