[PATCH] ax25: use GFP_KERNEL over GFP_ATOMIC where possible

[PATCH] ax25: use GFP_KERNEL over GFP_ATOMIC where possible

[Peter Lafreniere]

There are a few functions that can sleep that use GFP_ATOMIC.
Here we change allocations to use the more reliable GFP_KERNEL
flag.

ax25_dev_device_up() is only called during device setup, which is
done in user context. In addition, ax25_dev_device_up()
unconditionally calls ax25_register_dev_sysctl(), which already
allocates with GFP_KERNEL.

ax25_rt_add() is a static function that is only called from
ax25_rt_ioctl(), which must run in user context already due to
copy_from_user() usage.

Since it is allowed to sleep in both of these functions, here we
change the functions to use GFP_KERNEL to reduce unnecessary
out-of-memory errors.

Signed-off-by: Peter Lafreniere <pjlafren@mtu.edu>
---
 net/ax25/ax25_dev.c   | 4 ++--
 net/ax25/ax25_route.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c260..b264904980a8 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -52,7 +52,7 @@ void ax25_dev_device_up(struct net_device *dev)
 {
 	ax25_dev *ax25_dev;
 
-	if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_ATOMIC)) == NULL) {
+	if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n");
 		return;
 	}
@@ -60,7 +60,7 @@ void ax25_dev_device_up(struct net_device *dev)
 	refcount_set(&ax25_dev->refcount, 1);
 	dev->ax25_ptr     = ax25_dev;
 	ax25_dev->dev     = dev;
-	dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
+	dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
 	ax25_dev->forward = NULL;
 
 	ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;
diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c
index b7c4d656a94b..c77b848ccfc7 100644
--- a/net/ax25/ax25_route.c
+++ b/net/ax25/ax25_route.c
@@ -91,7 +91,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
 			kfree(ax25_rt->digipeat);
 			ax25_rt->digipeat = NULL;
 			if (route->digi_count != 0) {
-				if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) {
+				if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_KERNEL)) == NULL) {
 					write_unlock_bh(&ax25_route_lock);
 					ax25_dev_put(ax25_dev);
 					return -ENOMEM;
@@ -110,7 +110,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
 		ax25_rt = ax25_rt->next;
 	}
 
-	if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_ATOMIC)) == NULL) {
+	if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_KERNEL)) == NULL) {
 		write_unlock_bh(&ax25_route_lock);
 		ax25_dev_put(ax25_dev);
 		return -ENOMEM;
-- 
2.36.1

Re: [PATCH] ax25: use GFP_KERNEL over GFP_ATOMIC where possible

[Jakub Kicinski]

On Thu,  2 Jun 2022 07:21:38 -0400 Peter Lafreniere wrote:
> There are a few functions that can sleep that use GFP_ATOMIC.
> Here we change allocations to use the more reliable GFP_KERNEL
> flag.
> 
> ax25_dev_device_up() is only called during device setup, which is
> done in user context. In addition, ax25_dev_device_up()
> unconditionally calls ax25_register_dev_sysctl(), which already
> allocates with GFP_KERNEL.
> 
> ax25_rt_add() is a static function that is only called from
> ax25_rt_ioctl(), which must run in user context already due to
> copy_from_user() usage.
> 
> Since it is allowed to sleep in both of these functions, here we
> change the functions to use GFP_KERNEL to reduce unnecessary
> out-of-memory errors.
> 
> Signed-off-by: Peter Lafreniere <pjlafren@mtu.edu>

For merging into the Linux networking trees you'll have to repost next
week, this seems like an optimization and we're currently in the merge
window period where we only accept fixes.

Re: [PATCH] ax25: use GFP_KERNEL over GFP_ATOMIC where possible

[Dan Carpenter]

On Thu, Jun 02, 2022 at 07:21:38AM -0400, Peter Lafreniere wrote:
>  net/ax25/ax25_dev.c   | 4 ++--
>  net/ax25/ax25_route.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
> index d2a244e1c260..b264904980a8 100644
> --- a/net/ax25/ax25_dev.c
> +++ b/net/ax25/ax25_dev.c
> @@ -52,7 +52,7 @@ void ax25_dev_device_up(struct net_device *dev)
>  {
>  	ax25_dev *ax25_dev;
>  
> -	if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_ATOMIC)) == NULL) {
> +	if ((ax25_dev = kzalloc(sizeof(*ax25_dev), GFP_KERNEL)) == NULL) {
>  		printk(KERN_ERR "AX.25: ax25_dev_device_up - out of memory\n");
>  		return;
>  	}
> @@ -60,7 +60,7 @@ void ax25_dev_device_up(struct net_device *dev)
>  	refcount_set(&ax25_dev->refcount, 1);
>  	dev->ax25_ptr     = ax25_dev;
>  	ax25_dev->dev     = dev;
> -	dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC);
> +	dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
>  	ax25_dev->forward = NULL;
>  
>  	ax25_dev->values[AX25_VALUES_IPDEFMODE] = AX25_DEF_IPDEFMODE;

These two are fine.

> diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c
> index b7c4d656a94b..c77b848ccfc7 100644
> --- a/net/ax25/ax25_route.c
> +++ b/net/ax25/ax25_route.c
> @@ -91,7 +91,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
>  			kfree(ax25_rt->digipeat);
>  			ax25_rt->digipeat = NULL;
>  			if (route->digi_count != 0) {
> -				if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) {
> +				if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_KERNEL)) == NULL) {
>  					write_unlock_bh(&ax25_route_lock);

This write lock means it has to be GFP_ATOMIC.

>  					ax25_dev_put(ax25_dev);
>  					return -ENOMEM;
> @@ -110,7 +110,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route)
>  		ax25_rt = ax25_rt->next;
>  	}
>  
> -	if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_ATOMIC)) == NULL) {
> +	if ((ax25_rt = kmalloc(sizeof(ax25_route), GFP_KERNEL)) == NULL) {
>  		write_unlock_bh(&ax25_route_lock);

This change is buggy as well.

>  		ax25_dev_put(ax25_dev);
>  		return -ENOMEM;

regards,
dan carpenter