diff mbox series

[01/15] skbuff: introduce skb_pull_data

Message ID 20211201000215.1134831-2-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Delegated to: Luiz Von Dentz
Headers show
Series Rework parsing of HCI events | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 493, Passed: 492 (99.8%), Failed: 0, Not Run: 1
tedd_an/testrunnerrfcomm-tester success Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Luiz Augusto von Dentz Dec. 1, 2021, 12:02 a.m. UTC 
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.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Jakub Kicinski Dec. 1, 2021, 1:11 a.m. UTC | #1 
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.
Luiz Augusto von Dentz Dec. 1, 2021, 2:16 a.m. UTC | #2 
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.
Jakub Kicinski Dec. 1, 2021, 2:27 a.m. UTC | #3 
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!
Dan Carpenter Dec. 1, 2021, 5:20 a.m. UTC | #4 
Thanks for following up on this!  I had forgotten about it.  I'm really
happy this is going forward.

regards,
dan carpenter
Marcel Holtmann Dec. 1, 2021, 7:22 a.m. UTC | #5 
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
Jakub Kicinski Dec. 1, 2021, 3:22 p.m. UTC | #6 
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 mbox series

Patch

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