Re: [PATCH] Rename tsk_restore_flags() to current_restore_flags()

[Michal Hocko]

On Fri 07-04-17 10:03:26, NeilBrown wrote:
> 
> It is not safe for one thread to modify the ->flags
> of another thread as there is no locking that can protect
> the update.
> So tsk_restore_flags(), which takes a task pointer and modifies
> the flags, is an invitation to do the wrong thing.
> 
> All current users pass "current" as the task, so no developers have
> accepted that invitation.  It would be best to ensure it remains
> that way.
> 
> So rename tsk_restore_flags() to current_restore_flags() and don't
> pass in a task_struct pointer.  Always operate on current->flags.

yes this makes a lot of sense to me. 

It would be great if somebody cleaned up setting of ->flags to be always
set on current as well. This seems to be the case but there are places
where we do
	struct task_struct *tsk = current;
	[...]
	tsk->flags |= PF_$FOO


> Signed-off-by: NeilBrown <neilb@xxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
> 
> get_maintainer.pl suggested 35 recipients for this patch.
> I decided to trim that a bit ....
> 
> Can it just go through the tip tree??  It should be completely
> uncontroversial.
> 
> Thanks,
> NeilBrown
> 
>  drivers/block/nbd.c      | 2 +-
>  drivers/scsi/iscsi_tcp.c | 2 +-
>  fs/nfsd/vfs.c            | 2 +-
>  include/linux/sched.h    | 6 +++---
>  kernel/softirq.c         | 2 +-
>  net/core/dev.c           | 2 +-
>  net/core/sock.c          | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 7e4287bc19e5..04ec921727b7 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -239,7 +239,7 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send,
>  		}
>  	} while (msg_data_left(&msg));
>  
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  
>  	return result;
>  }
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 4228aba1f654..bbea8eac9abb 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -387,7 +387,7 @@ static int iscsi_sw_tcp_pdu_xmit(struct iscsi_task *task)
>  		rc = 0;
>  	}
>  
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  	return rc;
>  }
>  
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 19d50f600e8d..9aaf6ca77569 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1004,7 +1004,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
>  	else
>  		err = nfserrno(host_err);
>  	if (test_bit(RQ_LOCAL, &rqstp->rq_flags))
> -		tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
> +		current_restore_flags(pflags, PF_LESS_THROTTLE);
>  	return err;
>  }
>  
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee84fd43..0978fb74e45a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1286,10 +1286,10 @@ TASK_PFA_TEST(LMK_WAITING, lmk_waiting)
>  TASK_PFA_SET(LMK_WAITING, lmk_waiting)
>  
>  static inline void
> -tsk_restore_flags(struct task_struct *task, unsigned long orig_flags, unsigned long flags)
> +current_restore_flags(unsigned long orig_flags, unsigned long flags)
>  {
> -	task->flags &= ~flags;
> -	task->flags |= orig_flags & flags;
> +	current->flags &= ~flags;
> +	current->flags |= orig_flags & flags;
>  }
>  
>  extern int cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpumask *trial);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 744fa611cae0..4e09821f9d9e 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -309,7 +309,7 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
>  	account_irq_exit_time(current);
>  	__local_bh_enable(SOFTIRQ_OFFSET);
>  	WARN_ON_ONCE(in_interrupt());
> -	tsk_restore_flags(current, old_flags, PF_MEMALLOC);
> +	current_restore_flags(old_flags, PF_MEMALLOC);
>  }
>  
>  asmlinkage __visible void do_softirq(void)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7869ae3837ca..e8a366387a99 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4240,7 +4240,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
>  		 */
>  		current->flags |= PF_MEMALLOC;
>  		ret = __netif_receive_skb_core(skb, true);
> -		tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +		current_restore_flags(pflags, PF_MEMALLOC);
>  	} else
>  		ret = __netif_receive_skb_core(skb, false);
>  
> diff --git a/net/core/sock.c b/net/core/sock.c
> index a96d5f7a5734..20fb01b01ddb 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -325,7 +325,7 @@ int __sk_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  
>  	current->flags |= PF_MEMALLOC;
>  	ret = sk->sk_backlog_rcv(sk, skb);
> -	tsk_restore_flags(current, pflags, PF_MEMALLOC);
> +	current_restore_flags(pflags, PF_MEMALLOC);
>  
>  	return ret;
>  }
> -- 
> 2.12.2
> 



-- 
Michal Hocko
SUSE Labs